Repository navigation
Conversation
|
WordPress.com
Automattic for Agencies
|
|
Looks like one of the E2E tests has failed. You can fix them following these steps:
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
| // Render the glyph circle at the Figma size (16 -> 18) without affecting | ||
| // the 24px layout box. | ||
| svg { | ||
| transform: scale( calc( 18 / 16 ) ); |
There was a problem hiding this comment.
The default glyph size for the 24px viewBox is 16px (the icons bake the circle into the path). I initially kept everything at the library defaults, but the circles looked too small next to the Figma spec, so I scale them up to 18px here, which matches much better.
Still not ideal: the ring is baked into the glyph at 1.5 units, so after scaling it renders at ~1.7px, thicker than the 1px in Figma. Thinning it would require custom SVGs, which conflicts with the "use standard icons" feedback.
@ciampo, @nuriapenya, @jameskoster; I'm open to suggestions.
16px icon
18px icon
There was a problem hiding this comment.
Maybe the play here is to use a custom icon for the circle / half-circle icons.
I wonder if this will improve if/when we switch these icons to be stroke-based (instead of fill-based), since we should be able to scale them up while retaining the correct stroke width
cc @jasmussen
There was a problem hiding this comment.
Hi, thanks for the ping, and generally I'm always happy to respond to pings both on the system side (even if I'm not as versed as Jay is, he's out for a bit), and especially on the icon side as I've drawn a lot of them.
And if we need a custom icon, I can help make that happen. But if the question is: do we go 16px or 18px, there are a couple of near term answers, but one long term answer that if all goes well brings still more flexibility.
I'll start with the perspective. In WordPress/gutenberg#78808 I'm redrawing all the icons to be stroke-based, and to maintain their stroke-width when scaling down. That means you will be able to show the icons at any size and they will maintain their stroke-width. Theoretically it also opens the door to customizing the stroke-width to be just right for the context. We'll likely want to author some guidelines on best practices here, but for implementations like this, well that's what the PR exists to mitigate. The PR has been a while in review, but there's recent feedback, I'm hoping to rebas e
In the mean time, I find that 16px and 18 both alias reasonably, but theoretically 18 should interpolate better. Badge uses icons at 16px. But 18 seems fine too. Shown below, 16, 18, 24
If we zoom in the interpolation math of the 18 scale checks out:
But to get to the heart of it: you can use icons, we're happy to provide new ones, and stroke-based should bring lots of options to this specific option.
But, as Marco says, if doing something custom here (and honestly it could be CSS) is better for the time being, I'm not personally opposed to it.
There was a problem hiding this comment.
Thanks @jasmussen, that's really helpful, and great to hear the stroke-based redraw in WordPress/gutenberg#78808 is close.
In this PR I found and matched the proper existing @wordpress/icons for the Stepper indicators, replacing the previous ad-hoc approach. That part worked out well. The one new wrinkle is the circle ring: at the 18px size I settled on, it renders a touch thick. There's only one consumption of the Stepper component so far, and I'll share screenshots below showing how it looks with this PR's changes, which should make the tradeoff clearer.
I had a look through that PR, and the icons we use for the Stepper are already covered there (they're marked in black in the screenshot), which is a nice sign that this is well in hand.
Given that, I'd rather not invest in custom SVGs now. My proposal: keep the standard @wordpress/icons at 18px for this PR and accept the slightly thicker ring as a known, temporary tradeoff until we update to the stroke-based icons. At that point we can scale them cleanly and dial the stroke-width in to match the spec, with no custom maintenance burden.
Does that work for you ( @ciampo, @jasmussen, @nuriapenya ) as an interim step?
There was a problem hiding this comment.
Noting also as soon as the first set of stroke based icons land, I will immediately follow up with WordPress/gutenberg#78812, and then more for the rest of the icons.
There was a problem hiding this comment.
Good for me technically (I'll review the rest of the PR once it's out of draft), this feels like more of a design decision (cc @nuriapenya )
There was a problem hiding this comment.
I'd rather not invest in custom SVGs now. My proposal: keep the standard @wordpress/icons at 18px for this PR and accept the slightly thicker ring as a known, temporary tradeoff until we update to the stroke-based icons.
That's reasonable and works for me, too! Thanks @bogiii
There was a problem hiding this comment.
Noting by the way that stroke-based icons are on the way: WordPress/gutenberg#81274
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a trunk merge or rebase and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
…the number variant
… at 18px" This reverts commit abdc406.
The state icons draw a 16px circle inside their 24px viewBox. Draw the number-variant circle on the inner number span at the same 16px size (via a component token) so both variants render identically, while the 24px indicator layout box is unchanged. The number span only exists for statusless number-variant steps, so the completed/error guards on the circle rules are no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Scale the icon glyphs (which draw a 16px circle inside the 24px viewBox) by 18/16 and size the number-variant circle from the same 18px token, so both variants match the Figma circle size while the 24px layout box is unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Drop the separate icon-indicators describe block; the error-status test already asserts bullet icons render, and the indicatorVariant test now covers status steps rendering icons instead of numbers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The error indicator no longer renders a literal "!", so asserting its absence checks nothing; the svg assertion covers the icon rendering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e12922e to
3855fe3
Compare
ciampo
changed the title
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~809 bytes added 📈 [gzipped]) Details
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~773 bytes added 📈 [gzipped]) Details
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. |
This branch has not been deployed
5 participants
Part of the review feedback on #111036
Proposed Changes
@wordpress/iconsfor upcoming, current, completed, and error states.Why are these changes being made?
Testing Instructions
yarn workspace @automattic/ui storybook:start.indicatorVarianttonumber, and confirm that upcoming and current steps use 18px number circles while completed and error steps use 18px icons.Pre-merge Checklist
useCallback()for event handlers passed to child components?