chore: Apply one-theme panel colors#4598
Conversation
at-susie
requested review from
jkuelz and
jperals
and removed request for
a team and
jperals
at-susie
force-pushed
the
feat/onet-layout-color
branch
from
f6012a3 to
037ee18
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4598 +/- ##
=======================================
Coverage 97.50% 97.50%
=======================================
Files 948 948
Lines 30275 30275
Branches 11039 11039
=======================================
Hits 29520 29520
Misses 748 748
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| &:after { | ||
| background-color: awsui.$color-background-layout-toolbar; | ||
| background-color: awsui.$color-background-layout-panel; |
There was a problem hiding this comment.
why are we changing this back to panel? why can't we theme the toolbar token instead? wouldn't decreasing this specificity potentially break experiences that have themed the toolbar token?
There was a problem hiding this comment.
Good point — reverted this back to $color-background-layout-toolbar. The toolbar token is already defined and themed separately in app-layout-toolbar.ts, so there's no need to lower the specificity here.
|
|
||
| nav.navigation { | ||
| background-color: awsui.$color-background-container-content; | ||
| background-color: awsui.$color-background-layout-main; |
There was a problem hiding this comment.
why is this one not color-background-layout-panel too?
There was a problem hiding this comment.
Updated — but actually went a step further after reflection. Rather than switching to $color-background-layout-panel, we switched all three panel usages (VR navigation, toolbar navigation, toolbar drawer) to $color-background-layout-panel-content instead. The colorBackgroundLayoutPanelContent token's description already covers "side navigation and tools panel content background", which is exactly the right semantic fit here. This avoids introducing colorBackgroundLayoutPanel as a new usage when an existing token already fits.
| const tokens: StyleDictionary.ColorsDictionary = { | ||
| colorBackgroundLayoutMain: { light: '{colorNeutralGrey50}', dark: '{colorNeutralGrey1000}' }, | ||
| colorBackgroundLayoutPanel: { light: '{colorNeutralGrey50}', dark: '{colorNeutralGrey1000}' }, | ||
| colorBackgroundLayoutPanelContent: { light: '{colorNeutralGrey50}', dark: '{colorNeutralGrey1000}' }, |
There was a problem hiding this comment.
is colorBackgroundLayoutPanelContent actually used anywhere? if so, what is the difference between that and colorBackgroundLayoutPanel?
There was a problem hiding this comment.
Yes, it's now used — and that's actually the change we made here. We switched all three panel background usages (VR navigation, toolbar navigation, toolbar drawer) from $color-background-layout-panel to $color-background-layout-panel-content, since its description already covers "side navigation and tools panel content background". This means the two tokens are no longer redundant from a usage perspective. We then dropped colorBackgroundLayoutPanel from this context override (since nothing references it anymore) and replaced it with the appropriate colorBackgroundLayoutPanelContent override:
colorBackgroundLayoutPanelContent: { light: '{colorNeutralGrey50}', dark: '{colorNeutralGrey1000}' },
There was a problem hiding this comment.
Not directly related to this PR, but any references to colorIndigo in this file should really be replaced with colorPrimary or colorInfo.
There was a problem hiding this comment.
Done — all colorIndigo* references in colors.ts have been replaced with semantic equivalents: colorPrimary* for primary UI actions (controls, focus/selection borders, toggle selection states, slider, progress bar, accent, dropdown filter match) and colorInfo* for info-semantic tokens (status info text, info link colors, info indicator background). Also introduced colorInfo800 in the one-theme palette (#0033cc) to cover the info link hover state.
| colorBorderDividerSecondary: { light: '{colorNeutralGrey200}', dark: '{colorNeutralGrey800}' }, | ||
| colorBorderLayout: { light: '{colorNeutralGrey300}', dark: '{colorNeutralGrey750}' }, | ||
| colorGapGlobalDrawer: { light: '{colorNeutralGrey250}', dark: '{colorNeutralGrey1000}' }, | ||
| colorGapGlobalDrawer: { light: '{colorNeutralGrey250}', dark: '#000000' }, |
There was a problem hiding this comment.
you can use {colorBlack} instead of #000000
There was a problem hiding this comment.
Updated to use {colorBlack}.
|
|
||
| &.has-open-drawer { | ||
| background-color: awsui.$color-background-container-content; | ||
| &.one-theme { |
There was a problem hiding this comment.
Can we move .one-theme to the outmost layer and keep one theme specific styles in that block? I think it makes it easier in the future if everything is defined in one block.
4 participants
Description
This PR updates panel color and shadow tokens for one-theme.
Adds shadow tokens and panel color support to the one-theme style dictionary, and updates app-layout component styles to use the new tokens.
Related links, issue #, if available: n/a
How has this been tested?
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.