components: Use updated range styles #33824
Conversation
sarayourfriend
added
[Type] Enhancement
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +74 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
sarayourfriend
changed the title
|
Very nice work, this is working well and is close! This is what I see:
Zooming in though, there are a few small things:
The tooltip isn't perfectly centered above the knob. That's probably fine, but if easy to fix would be a nice bit of polish. The focus style is square, though, and 1px. Ideally it's circular like the knob, and 1.5px. This is a bit gnarly as it means we can't use either The focus style on the current toggle is actually off (it's 2px instead of 1.5px), but the same CSS could potentially be used: I realize that creates a white halo on the inside to emulate the outset. I wonder if we can use Note that the 2px solid transparent outline is necessary if we use Let me know if you'd like me to push some tweaks to the branch directly, happy to take a stab. |
There was a problem hiding this comment.
Thank you @sarayourfriend for working on this!
Here's some feedback:
Which Figma page?
Global Styles figma
Which page in the Figma document should be used as the reference?
Track color
Compared to what's currently on production, the track seems to show a regression: the track color is always set to the "active" primary color, even for the portion of the track that has values greater than the thumb:
| Production | This PR |
|---|---|
 |
 |
Tooltip centering
Like @jasmussen pointed out, I also noticed that the tooltip isn't aligned with the thumb.
I played around with the code and I noticed that bringing back the thumbSize in packages/components/src/range-control/styles/range-control-styles.js to its original value of 20 seems to fix the alignment issue — which makes me believe that there are some other hardcoded values in the styles that should also change when the thumbSize value changes.
Focus styles
This one also has been pointed out by @jasmussen — it'd be great if we could have a circular outline.
Another solution for achieving that may be to add a pseudo-element to the thumb (using ::before or ::after) and styling it with a transparent background and a border.
"With Icon" Storybook examples
Finally, I can't see any icons in the "With Icon Before" and "With Icon After" storybook examples. Are they working as intended?
| @@ -55,7 +55,7 @@ function RangeControl( | |||
| onFocus = noop, | |||
| onMouseMove = noop, | |||
| onMouseLeave = noop, | |||
| railColor, | |||
| railColor = 'var( --wp-admin-theme-color )', | |||
There was a problem hiding this comment.
Not introduced by this PR, but I got a bit confused with the rail vs track naming. Do they refer to the same part of the UI ?
The README even has an Anatomy part where it specifically highlights the "Track", but then there is a prop called railColor, a file called rail.js and, in the styles, there are CSS snippets refererrin to both the track and the rail.
Should we try to use only one term everywhere?
There was a problem hiding this comment.
Rail is the "filled in" side and "track" is the background.
There was a problem hiding this comment.
Should we add this explanation to the README, maybe adding "Rail" to the "Anatomy" section?
|
So interesting, if you check my screenshot of the focus styles... they are circular on Firefox. I'll have to see if Chrome is doing something odd 🤷 |
It appears that the border-radius of the element affects the outline radius on Firefox, but not on Chrome or Safari:
Not only that, but Firefox even supports subpixel rendering, allowing 1.5px outline widths:
You can test in this codepen: https://codepen.io/joen/pen/LYyJEoL?editors=1100 Since we need both the 1.5px stroke width, and the radius, it seems we need to paint them using box-shadow for the time being until Chrome and Safari catch up. Painting it on a pseudo element seems like it might be a good solution. |
|
Same thing with the track color. From the Screenshots they work fine in Firefox. Seems to be a Chrome thing. I guess I'm glad some people are still using Chrome to catch these shortcomings in that browser 🤷 |
@ciampo I can't reproduce this on Chrome? Any hints? |
|
@ciampo The icon appears to be an existing bug: https://wordpress.github.io/gutenberg/?path=/story/components-rangecontrol--with-icon-after |
Not really, I believe I followed the standard steps:
Thank you for the update — we can look into this in a separate PR |
Aha, this was the key part, it has something to do with the initial value configuration. I can reproduce it in Firefox now as well, thanks! |
|
Focus style looks great now, thank you!
I didn't even notice before, but I can also reproduce the fully blue rail color. Just insert a gallery in the post editor and choose the number of columns in the inspector. |
|
Okay the rail color issue is fixed now. I don't really understand why this fixes it though 🤷 |
|
Can confirm, sweet:
Going into super nitpicky details mode, there's a 10px left margin on the item, at least when it's used for the gallery block. Can we remove it?
In extreme detail mode (in fact I had to nearest-neightbor upscale a screenshot to be sure), the range track should be pill shaped, but for whatever reason it looks like it has a 1px border radius:
I can see the 3px border radius right there in the inspector, it should for all intents and purposes render the item perfectly pill shaped. But it isn't working for me, and I've no idea why. As a sidenote or little CSS trick I picked up: while a 6px tall rectangle given a 3px border radius should be perfectly pill shaped. However a 6, 7, 10, 12, or 9999px tall rectangle given a 9999px will also be perfectly pill-shaped. So long as it's a finite pixel value, border-radius will simply pill-shape the item. While it shouldn't matter in this case, it's a nice simple trick to keep pill-shape CSS simple not needing any math. |
Curious! What was the reason to add a default value in the first place? |
I don't remember. |
| @@ -83,7 +81,7 @@ export const Rail = styled.span` | |||
| position: absolute; | |||
| margin-top: 14px; | |||
| top: 0; | |||
| border-radius: 3px; | |||
| border-radius: 9999px; | |||
There was a problem hiding this comment.
Thanks for doing that!
As it turns out, there appears to be a browser error here, which I will codepen and report to the Chrome team. Spot the odd-one out (open in new tab to see at 1x):
|
@ciampo Do you have a chance to re-review and approve if possible? |
There was a problem hiding this comment.
It's looking great, @sarayourfriend !
There are only a few minor things left to improve, feel free to merge this PR after addressing them.
Thumb alignment with marks
It looks like the thumb is not centered above the marks. Probably some computation around how the marks are displayed also depends on the thumbSize
Thumb color when disabled
This was not necessary with the previous design, since the thumb gray and white. But, now that the thumb is blue, I wonder if we should also change to gray when the input is disabled:
Bonus round: Rail documentation
It'd be great if we added a list item for the Rail under the Anatomy section in the README.
sarayourfriend
force-pushed
the
add/updated-range-control-styles
branch
from
04da49b
to
1060532
Compare
priethor
added
the
[Focus] Accessibility (a11y)
Description
Use the updated range styles from the Global Styles figma: https://www.figma.com/file/oEkcAyhIvPFMVEAO8EImvA/Global-Styles
How has this been tested?
Run storybook and check out the range control page:
iframe.html?id=components-rangecontrol--defaultScreenshots
Unfocused
Focused
Types of changes
Style changes (new feature???)
Checklist:
*.native.jsfiles for terms that need renaming or removal).