Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4395 +/- ##
=======================================
Coverage 97.45% 97.45%
=======================================
Files 909 909
Lines 26638 26638
Branches 9615 9616 +1
=======================================
Hits 25959 25959
+ Misses 673 636 -37
- Partials 6 43 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/app-layout/visual-refresh-toolbar/toolbar/trigger-button/styles.scss
Outdated
Show resolved
Hide resolved
src/app-layout/visual-refresh-toolbar/toolbar/trigger-button/styles.scss
Outdated
Show resolved
Hide resolved
| colorTextButtonInlineIconDefault: { | ||
| description: 'The default color of inline button icons.', | ||
| themeable: true, | ||
| public: true, | ||
| }, | ||
| colorTextButtonInlineIconDisabled: { | ||
| description: 'The color of inline button icons in disabled state.', | ||
| themeable: true, | ||
| public: true, | ||
| }, | ||
| colorTextButtonInlineIconHover: { | ||
| description: 'The color of inline button icons in hover state.', | ||
| themeable: true, | ||
| public: true, | ||
| }, |
There was a problem hiding this comment.
These weren't explicitly mentioned in the PR description, so just making sure they are supposed to be here.
There was a problem hiding this comment.
These existing tokens currently defaults to colorTextLinkXxx, which are currently not themeable, but need to be individually themeable.
| public: true, | ||
| themeable: false, | ||
| }, | ||
| spaceFieldHorizontal: { |
There was a problem hiding this comment.
Do we need a vertical one for this as well since buttons and fields should be the same height?
There was a problem hiding this comment.
Changing vertical padding doesn't change the height of inputs, since the height of inputs is specified using the token sizeVerticalInput, so we might need to make it themeable. I'll look into it.
There was a problem hiding this comment.
Update:
- I made the
sizeVerticalInputtoken themeable - I introduced
spaceControlVerticaltoken to be able to individually theme
There was a problem hiding this comment.
With the switch from 'inline-sizetomin-inline-sizechange, is thesizeVerticalInput` token redundant?
There was a problem hiding this comment.
The sizeVerticalInput is still needed as min-block-size. We can also change from sizeVerticalInput to something like sizeMinVerticalInput to be explicit. WDYT?
ddf6da3 to
08b615a
Compare
| inset-block-start: 1px; | ||
| inset-block-start: -1px; | ||
| inset-inline-end: -1px; | ||
| outline: solid 2px awsui.$color-background-layout-panel-content; |
There was a problem hiding this comment.
perhaps this deserves its own token for toolbar-background?
There was a problem hiding this comment.
It can, but that feels a bit out of scope for this PR. Happy to revisit whenever needed.
| spaceAlertVertical: '{spaceFlashbarVertical}', | ||
| spaceButtonFocusOutlineGutter: '4px', | ||
| spaceButtonHorizontal: '{spaceScaledL}', | ||
| spaceButtonVertical: '{spaceScaledXxs}', |
There was a problem hiding this comment.
should this default to spaceControlVertical? Like spaceButtonVertical: '{spaceControlVertical}' since they should be the same most of the time?
There was a problem hiding this comment.
I would rather keep it separated from spaceControlVertical as button can have different stroke-widths And depending on that they need to adjust vertical padding independently. So they are different.
| $control-padding-vertical: awsui.$space-control-vertical; | ||
| $control-padding-horizontal: awsui.$space-field-horizontal; |
There was a problem hiding this comment.
I know these are kinda inconsistent in general, but since the horizontal token uses "field" naming, should we keep it consistent? awsui.$space-field-vertical?
There was a problem hiding this comment.
Updated to be space-field-vertical
|
|
||
| @include styles.font-body-m; | ||
| block-size: awsui.$size-vertical-input; | ||
| min-block-size: awsui.$size-vertical-input; |
There was a problem hiding this comment.
Doesn't this change make the field respect awsui.$space-control-vertical when themed? If so, do we need to expose both? I would also double check that this change doesn't have any adverse effects across browsers or on fields that have multiple lines of text (e.g. dropdowns with additional metadata). IIRC, this used to cause issues in Safari, but it could be fixed by now.
There was a problem hiding this comment.
If we keep the block-size, it will not respect the awsui.$space-control-vertical when themed. That's why we need to make it min-block-size. This way, the min-block-size works as a guardrail while the awsui.$space-control-vertical is themeable.
There was a problem hiding this comment.
But it makes sense to double-check different examples.
| &-multiselect-wrapper { | ||
| position: relative; | ||
| block-size: awsui.$size-vertical-input; | ||
| min-block-size: awsui.$size-vertical-input; |
There was a problem hiding this comment.
Same comment here about the cross-browser behavior and redundant tokens.
There was a problem hiding this comment.
As mentioned in the above reply, we would rather need to make it min-block-size
| public: true, | ||
| themeable: false, | ||
| }, | ||
| spaceFieldHorizontal: { |
There was a problem hiding this comment.
With the switch from 'inline-sizetomin-inline-sizechange, is thesizeVerticalInput` token redundant?
Description
This PR adds independently themeable design tokens for the button component. Specifically it introduces dedicated color tokens for the link button variant to achieve parity with normal and primary variants, and adds a new spaceButtonVertical spacing token so button padding is themeable in both directions. No visual changes — all new tokens default to the previous values.
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.