feat: implement style api for segmented control component #4068
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4068 +/- ##
=======================================
Coverage 97.12% 97.12%
=======================================
Files 863 864 +1
Lines 25294 25318 +24
Branches 9119 9136 +17
=======================================
+ Hits 24566 24590 +24
Misses 722 722
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mxschll
force-pushed
the
dev-v3-schomax-segmented-control
branch
from
92d5cf0 to
a1949e4
Compare
mxschll
requested review from
gethinwebster and
jperals
and removed request for
a team and
gethinwebster
| }, | ||
| }; | ||
|
|
||
| const iconSvg = ( |
There was a problem hiding this comment.
Nitpick: is the custom SVG relevant to this feature?
There was a problem hiding this comment.
It's out of scope for now.
There was a problem hiding this comment.
What I mean is whether it makes sense to include this in the dev page for testing. Can using iconSvg affect the custom styles (or the other way around) in any way?
I wonder if the dev page can be even further simplified if we only include the prop values that are relevant to test the custom styles feature.
Not a blocker though.
There was a problem hiding this comment.
You are right, thanks for pointing that out.
|
|
||
| exports[`getSegmentedControlSegmentStyles handles all possible style configurations 1`] = ` | ||
| { | ||
| "--awsui-style-background-active-d43v8n": undefined, |
There was a problem hiding this comment.
What do these hashes depend on? Can they change due to something unrelated?
There was a problem hiding this comment.
The hash is dependent on the list in custom-css-properties.js and get generated using this function:
const hash = getHashDigest(Buffer.from(JSON.stringify(customCssPropertiesList)), 'md5', 'base36', 6);
They will change if we introduce a new custom property, which is rarely the case. I agree that this is not ideal, but I think we can live with that for now until we have a more general solution for testing the style api. What do you think?
There was a problem hiding this comment.
I have doubts whether we need these snapshot tests, provided that we have visual regression tests in the pipeline.
Even if adding new custom properties is rare, when we do it these tests will fail unexpectedly, but the end user experience will not change —in this sense, the visual result is what matters here.
This said, not a blocker.
There was a problem hiding this comment.
Yeah, I think we might need to think about a more unified testing strategy for the style api overall. Maybe introducing a small util function would help.
2 participants
Description
Extends the segmented control component with a style API for customization, allowing developers to override default component styles.
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.