feat: Update token component styles for one-theme#4633
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 #4633 +/- ##
=======================================
Coverage 97.51% 97.51%
=======================================
Files 948 948
Lines 30353 30360 +7
Branches 11072 11074 +2
=======================================
+ Hits 29600 29607 +7
Misses 746 746
Partials 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| expect(inlineIcon).toHaveClass(styles['icon-inline']); | ||
| }); | ||
|
|
||
| test('overrides icon size to x-small for inline variant in OneTheme', () => { |
There was a problem hiding this comment.
suggestion: let's create a section "One theme" under describe and set document.body.classList.add('awsui-one-theme') before each and document.body.classList.remove('awsui-one-theme') after each test.
There was a problem hiding this comment.
Great suggestion. I moved the OneTheme test out of Icons describe block into its own describe('One theme', ...) with beforeEach/afterEach for class management.
| * | ||
| * When `variant="inline"`, icon size should be `small`. | ||
| * | ||
| * For the OneTheme with `variant="inline"`, the icon size will automatically be overridden to `x-small`. |
There was a problem hiding this comment.
This strings will end up on the external website as well. Is that intentional?
There was a problem hiding this comment.
Removed the OneTheme-specific sentence from the icon prop.
| }); | ||
|
|
||
| const sizedIcon = (iconNode: React.ReactNode) => { | ||
| if (isInline && isOneTheme && React.isValidElement(iconNode)) { |
There was a problem hiding this comment.
You also need to check if the iconNode is an instance of the Icon component, not just any react component
There was a problem hiding this comment.
Added iconNode.type === InternalIcon so sizedIcon only resizes actual InternalIcon elements
|
|
||
| const sizedIcon = (iconNode: React.ReactNode) => { | ||
| if (isInline && isOneTheme && React.isValidElement(iconNode)) { | ||
| return React.cloneElement(iconNode as React.ReactElement<{ size?: string }>, { size: 'x-small' }); |
There was a problem hiding this comment.
Why do you need to cast the data here?
There was a problem hiding this comment.
Good call. Removed the as React.ReactElement<{ size?: string }> cast. Once the type guard iconNode.type === InternalIcon is in place, TypeScript can infer enough to make React.cloneElement work cleanly.
| testUtilStyles['dismiss-button'], | ||
| inline && styles['dismiss-button-inline'] | ||
| inline && styles['dismiss-button-inline'], | ||
| isOneTheme && styles['one-theme'] |
There was a problem hiding this comment.
Does it make sense to declare this variable in every component that needs one-theme styles?
let's create a new one-theme mixin here (similar to what we do for dark mode) and use it for css only, as it's less verbose and more scalable
https://github.com/cloudscape-design/components/blob/main/src/internal/styles/utils/theming.scss#L5
There was a problem hiding this comment.
That makes sense. I added the one-theme-only mixin in the src/internal/styles/utils/theming.scss.
This way I don't have to have isOneTheme && styles['one-theme'].
2 participants
Description
This PR:
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.