-
Notifications
You must be signed in to change notification settings - Fork 204
chore: Themeable icon stroke width #4079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
=======================================
Coverage 97.12% 97.12%
=======================================
Files 864 864
Lines 25325 25325
Branches 9137 9137
=======================================
Hits 24597 24597
Misses 722 722
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const [strokeSmall, setStrokeSmall] = useState<string>('1'); | ||
| const [strokeNormal, setStrokeNormal] = useState<string>('1'); | ||
| const [strokeMedium, setStrokeMedium] = useState<string>('1'); | ||
| const [strokeBig, setStrokeBig] = useState<string>('1.5'); | ||
| const [strokeLarge, setStrokeLarge] = useState<string>('2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does values are not aligned with the default values like 2px, 3px and 4px. Is this on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I will refine the UIs overall in the example page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general question here to the formula at the end. The stroke width is divided by scale factor, which contradicts the token definition, for example for big would the calc: 3px/ 2 = 1.5px but it is defined as 3px in tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking. The Cloudscape icon component provides the size property ranging from small to large. The way of handling the different icon sizes is to scale up and down by changing the CSS size property. This allows us to manage only one single icon svg resource and use it for different sizes.
However, when we scale the icon up (for example, 2x for "big" size), the stroke width would also scale proportionally, making it too thick. To maintain consistent visual weight across all icon sizes, we need to compensate by dividing the stroke width by the scale factor to make it render with the expected visual stroke widths, for example "big" size:
- The icon itself is scaled up by 2x (from 16px to 32px)
- The token borderWidthIconBig is defined as 3px
- In the CSS, we apply: stroke-width: calc(3px / 2) = 1.5px
- When the browser renders the scaled-up icon (2x), the effective stroke width becomes: 1.5px × 2 = 3px
| value={strokeSmall} | ||
| onChange={evt => { | ||
| const numValue = parseFloat(evt.detail.value); | ||
| if (!isNaN(numValue) && numValue >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to much but we could add here min/max constraints
Description
As part of the iconography extension project, this PR introduces themeable border tokens that define stroke widths for each icon size variant. These tokens provide the flexibility needed to customize icon styling.
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.