chore: apply one theme styles to side nav#4670
Conversation
mxschll
force-pushed
the
dev-v3-schomax-sidenav
branch
from
41023b3 to
a753a6b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4670 +/- ##
=======================================
Coverage 97.57% 97.57%
=======================================
Files 948 948
Lines 30489 30498 +9
Branches 11149 11157 +8
=======================================
+ Hits 29749 29758 +9
- Misses 693 733 +40
+ Partials 47 7 -40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| setHighlighted(value); | ||
| if (value) { | ||
| const { reset } = applyTheme({ | ||
| theme: { |
There was a problem hiding this comment.
Even though this is just a dev page, we don't want to see the themed version but the actual version of the side nav in one theme. Hence, removed this part.
|
|
||
| // Emits a divider as its own list segment (dividers break the <ul> grouping). | ||
| function pushDivider() { | ||
| lists[lists.length] = { |
There was a problem hiding this comment.
No reason, this was carried over from the original divider code. Changing it.
| padding-inline: $item-padding-inline; | ||
| list-style: none; | ||
| @include styles.with-motion { | ||
| transition: margin awsui.$motion-duration-expressive awsui.$motion-easing-responsive; |
There was a problem hiding this comment.
This was added with the collapsed state side nav, but we don't want this animation here.
| colorTextSideNavigationItemActiveCollapsed: { light: '{colorNeutral100}', dark: '{colorNeutral1000}' }, | ||
| colorTextSideNavigationItemDefault: { light: '{colorTextBodyDefault}', dark: '{colorTextBodyDefault}' }, | ||
| colorBackgroundSideNavigationItemActive: { light: 'transparent', dark: 'transparent' }, | ||
| colorBackgroundSideNavigationItemActiveCollapsed: { light: '{colorPrimary500}', dark: '{colorPrimary500}' }, |
There was a problem hiding this comment.
As long as we provide this collapsing side nav feature, we should apply the same background treatment for VR as well, otherwise only changing the icon color only when the side-nav is collapsed doesn't meet the a11y requirement for VR.
There was a problem hiding this comment.
Thanks, that's true. I didn't consider it as we only looked at the one theme use case, but you're right.
| // The inner <ul> carries the group label so list semantics are preserved | ||
| // for screen readers even when the visual header is hidden. | ||
| function pushCollapsedGroup(children: ReadonlyArray<SideNavigationProps.Item>, label: string) { | ||
| function pushCollapsedGroup( |
There was a problem hiding this comment.
There might be a bug: When selecting an item in the Projects category and collapse the side-nav, the Projects category icon is not highlighted, whereas other category works (See the video recording)
Screen.Recording.2026-07-01.at.12.58.12.mov
There was a problem hiding this comment.
I think we will have guidance not to use expandable link groups in collapsible side nav. Will confirm.
at-susie
left a comment
at-susie
left a comment
There was a problem hiding this comment.
Left some comments inline. The highlighting style when collapsed is needed for VR as well otherwise it's not accessible.
3 participants
Description
Applies one theme related changes to collapsible sidenav.
Related links, issue #, if available: n/a
How has this been tested?
Locally.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.