chore: Introduce visualAccent prop to box#4679
Conversation
at-susie
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4679 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 950 950
Lines 30655 30660 +5
Branches 11234 11240 +6
=======================================
+ Hits 29919 29924 +5
+ Misses 729 689 -40
- Partials 7 47 +40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
at-susie
force-pushed
the
onetheme/box-accent
branch
from
d9ccc2b to
0f8d417
Compare
| colorBackgroundCodeView: { light: '#f8f8f8', dark: '#282c34' }, | ||
|
|
||
| // ── Visual accent ─────────────────────────────────── | ||
| colorBackgroundAccentRed: { light: '{colorError100}', dark: 'rgba(255, 61, 61, 0.2)' }, |
There was a problem hiding this comment.
Why do we need these tokens in visual refresh? This is a 1T feature.
There was a problem hiding this comment.
Moving these to 1T style-dictionary
There was a problem hiding this comment.
I looked into moving the values into one-theme, but it turned out that they need to stay rooted in visual-refresh as the default package is built with classic as the primary theme (which inherits from visual-refresh), while one-theme is only ever a secondary theme.
| return tagOverride; | ||
| } | ||
|
|
||
| if (hasVisualAccent) { |
There was a problem hiding this comment.
You can merge this: if (hasVisualAccent || variant === 'awsui-value-large')
There was a problem hiding this comment.
Why does it have to be a span if hasVisualAccent === true?
There was a problem hiding this comment.
This was a leftover from an old experiment. Updated not to make it span by default when hasVisualAccent === true. Now it just follows the box variant
|
|
||
| export default function StyleBoxPage() { | ||
| return ( | ||
| <div style={{ display: 'flex', justifyContent: 'center' }}> |
There was a problem hiding this comment.
Can we use SimplePage and have a screenshot area inside, so we can take screenshot tests?
|
|
||
| const accentAspectRatio = visualAccent?.aspectRatio ?? 'auto'; | ||
| const accentBorderRadius = visualAccent?.borderRadius; | ||
| const isRadiusKeyword = accentBorderRadius !== undefined && RADIUS_KEYWORDS.includes(accentBorderRadius); |
There was a problem hiding this comment.
Can we use types instead of having an array RADIUS_KEYWORDS? That is a bit awkward and I don't think we do that elsewhere?
There was a problem hiding this comment.
It's also hard to debug, if I pass an arbitrary string but nothing happens, I can't know why.
There was a problem hiding this comment.
Good call. borderRadius is now a strict keyword union (xxxs…xxxl plus full), matching padding/margin. Dropped the runtime keyword array and check. Invalid values are caught by the type system now.
| padding-inline: awsui.$space-xs; | ||
| // Force the element to be a square regardless of content dimensions. | ||
| // The width is constrained to match the height (content + padding). | ||
| aspect-ratio: 1 / 1; |
There was a problem hiding this comment.
With long text, the element grows very large. How should this feature behave with text, or is not not supposed to be used with text?
There was a problem hiding this comment.
Such extreme things can happen in buttons today as well. We will provide guidances
| // the stylesheet through a custom property, which the base `.visual-accent` rule reads and | ||
| // applies to all four corners. When unset, no custom property is set and the base rule falls | ||
| // back to a `0` border radius. | ||
| const accentStyle = |
There was a problem hiding this comment.
Nit: you could remove this const and move this directly to the style prop since it's not used anywhere else. Also, related to the comment above: it's weird to have this isRadiusKeyword check, instead of using types.
There was a problem hiding this comment.
Resolved by the same change. There is no more accentStyle const or isRadiusKeyword branch. The keyword maps directly to a .visual-accent-radius-* class.
2 participants
Description
This PR introduces a new property
visualAccentprop to the box component. The new prop allows builders to visually emphasize specific elements through a curated set of background and content color combinations.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.