UnitControl: Improve interactions + add unit parsing #22329
Conversation
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
|
Size Change: +9.98 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
|
||
| ### isFloatingLabel | ||
|
|
||
| If true, the `label` will render with a floating interaction. |
There was a problem hiding this comment.
This description seems a bit vague; even I'm not entirely sure what it means. I assume it means the label appears inside the input until the input is focused? Also, is this prop even a good idea, accessibility-wise?
Also, all this type info belongs in a JSDoc comment, doesn't it?
There was a problem hiding this comment.
Thanks for noticing this! I'm not sure how best to describe this interaction/design.
It works like the TextFields from Material UI:
https://material-ui.com/components/text-fields/
From a design perspective, it offers more versatility, especially if controls need to be combined in cramped spaces.
As for as accessibility goes, how its setup, I believe it it acceptable from both a motion perspective and aria label + HTML markup perspective.
cc'ing some a11y folks!
@enriquesanchez / @diegohaz Are there any concerns for having floating labels in Inputs like these?
https://material-ui.com/components/text-fields/
Thank you!
There was a problem hiding this comment.
I don't have strong opinions about that. But there's this article with arguments against floating labels: Floating labels are problematic
And this other one is a response to that: Are float labels really that problematic after all?
There was a problem hiding this comment.
I always check Adam Silver's articles when in doubt about forms and a11y. That first link Diego shared is from him :)
- Sorry if this is obvious, but where is this floating label going to be used? Only on this control or on all input fields?
- I don't see the floating label in the gif example above, but how would it look like when the floating label is longer than the field itself?
There was a problem hiding this comment.
@diegohaz + @enriquesanchez Thanks for replying!
where is this floating label going to be used? Only on this control or on all input fields?
It's situational! By default, floating labels are disabled. They're useful if the input is narrower or used in compact spaces. For example, within a smaller Popover.
but how would it look like when the floating label is longer than the field itself?
Ah! the GIF above does not have a floating label. The best example of this interaction would be from the Material TextField demo:
https://material-ui.com/components/text-fields/
My screen capture -> GIF software doesn't do a good job at capturing the various stages.
The key frames work like this ✨
No value, label works like a placeholder:
No value, focused, the label floats up:
With value, the label stays up:
There was a problem hiding this comment.
Ah! Gotcha. I misread. My apologies. There isn't handling of this at the moment.
The Material TextField doesn't handle this use case too well either:
https://codesandbox.io/s/material-demo-e49dz
I can try some truncation techniques really quickly. I don't think this should be a blocker though.
There was a problem hiding this comment.
The big issue is that input labels have to be translated, and some languages will use much longer words than others. An input with a floating label of "left" might be correctly sized, but once you translate that to another language, it may suddenly be twice as long.
WordPress generally prefers decisions over options. If there's not a really good reason to provide an alternate input style, we shouldn't provide it. I think it's okay to merge this PR since the InputControl will be experimental, but I don't think we should change the component to stable until we know whether the floating label options should be kept or not.
There was a problem hiding this comment.
This is the best I could do given the (surprisingly) limited CSS rendering control over <fieldset>.
I don't think we should change the component to stable until we know whether the floating label options should be kept or not
Yup! I agree. We can also drop the floating label feature if it's causing issues.
There was a problem hiding this comment.
Thanks @ItsJonQ! I was trying to make a point with my question, apologies if it wasn't clear 😃.
I feel that at this point there's some uncertainty re: floating labels (a11y, localization, placement, etc.) that my recommendation is to stick to regular ol' fashioned text labels.
There was a problem hiding this comment.
I feel that at this point there's some uncertainty re: floating labels (a11y, localization, placement, etc.) that my recommendation is to stick to regular ol' fashioned text labels.
I agree! Ol' fashion text labels are default 😊
The floating labels are a new feature, which can be experimentally trialed in certain scenarios
| /** | ||
| * External dependencies | ||
| */ | ||
| import { noop } from 'lodash'; |
There was a problem hiding this comment.
I've seen this code pattern used in several places, but given that we can now use optional chaining, I think that instead of doing this, we could just let the functions be undefined by default. I'm just guessing (and could be completely wrong about this), but it seems like that would perform slightly better. To avoid calling functions that are undefined, we would use optional chaining like so:
onBlur?.( event )`Again, this is just an idea; it probably doesn't matter much either way.
There was a problem hiding this comment.
Interesting idea! I thought about it for a bit (considering optional chaining is a new thing)
From my experience in the React library/module space, I've found it smoother for callbacks to fire without guards. It kept the code tidier and there was less to think about.
That being said, optional chaining does tidy it up a lot.
I'm perhaps biased in that I still prefer noop callbacks.
I'm open to using optional chaining instead, if that's what others prefer 👍
ItsJonQ
force-pushed
the
try/improve-unit-control
branch
from
e46992f
to
920060b
Compare
|
Thanks for the feedback all! I'll merge this once Travis is green and happy ✨ |
| @@ -61,7 +61,7 @@ export function parseUnit( initialValue, units = CSS_UNITS ) { | |||
| unit = unit.toLowerCase(); | |||
|
|
|||
| if ( hasUnits( units ) ) { | |||
| const match = units.find( ( item ) => item.value === unit ); | |||
| const match = keyBy( units, 'value' )[ unit ]; | |||
There was a problem hiding this comment.
@ItsJonQ: Ah, I wasn't clear, but I meant that a keyed version of units could be kept where it can be reused — so possibly as a module constant. Otherwise we're still iterating over the collection (and in this case doing extra work) every time!
There was a problem hiding this comment.
@mcsf Ah, gotcha! It's a little tricky, as the units coming in may not be constant. Ultimately, this is determined by the add_theme_support config within the context of Gutenberg.
We could memoize it? I'm wondering if it's worth it, considering this operation is quite small (within the grand scheme of things)
There was a problem hiding this comment.
We could memoise, yeah, but I don't want to take more of your time on this now, so I suggest reverting to the initial array and find situation, and we can improve from there. :)
There was a problem hiding this comment.
Sounds good! Thank you @mcsf !!
This update adds rich features and interactions to the UnitControl component. Originally, this work was in support of adding padding controls to the Cover block. This update abstracts just the UnitControls from that effort. UnitControl and NumberControl are now powered by a new lower level primitive: InputControl. InputControl handles the majority of the interaction and event logic. Components using it, like NumberControl, are able to tap into the state flow and add features like "drag to update".
Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>
Also improve Safari placeholder vertical alignment
This reverts commit 8fdc6e8.
|
Rebasing with latest |
ItsJonQ
force-pushed
the
try/improve-unit-control
branch
from
3258dcb
to
637a62e
Compare
|
Accidentally clicked "Close and comment" 🙈 |
|
Hmm... looks like Travis has been failing the E2E (not just this PR, but for others as well) |
|
Yay! Travis is happy! Merging :D |
|
Thanks again everyone! I can't wait to use these 🎉 |
|
Late to the party. I came across this while testing the new Navigation page.
I'm not sure I understand why a fieldset + legend elements are used for this pattern. They're semantic elements meant to group logically-related form controls. The group name is given by the legend element. Instead, in this case:
This is confusing: why we should render a fieldset when it's not needed? @ItsJonQ can you please shed some light on the reason why a fieldset + legend elements are used here? Regarding the "floating labels" pattern: in general, anything that "moves" on the screen without user activation is potentially confusing and triggers cognitive load. I'm not sure this pattern should be used without an in-depth discussion between the accessibility and design teams. In any case, there are better ways to implement "floating labels" that make sure the input is labelled. See for example what GMail does:
where it's of fundamental importance that the visible text matches the input aria-label. |
@afercia Hi there! Thank you for digging in! I had used I'll follow up with improvements on this as soon as I can. Would it be okay if I |
Sure. I'd consider to also post a quick heads up on the Slack accessibility channel so that if I'm not available, someone else can provide feedback. Thanks! ❤️ |
|
@afercia Will do! Will also be sure to tag the PR with "Needs Accessibility Feedback" |
|
I just noticed that now that the styles from |
@afercia I noticed this too when working on the fix. This has been resolved! (Good timing, I'm writing the PR as we speak!) |
|
Ah good! I was just writing the new issue :D Too many |
Description
(GIF: Demo of dragging UnitControl to update Cover height)
This update adds rich features and interactions to the UnitControl component. Originally, this work was in support of adding padding controls to the Cover block. This update abstracts just the UnitControls from that effort.
Notable features for UnitControl include:
10pxis parsed and understood as[10, 'px']UnitControl and NumberControl are now powered by a new lower level primitive: InputControl.
InputControl handles the majority of the interaction and event logic. Components using it, like NumberControl, are able to tap into the state flow and add features like "drag to update".
Notable features for InputControl include:
How has this been tested?
How to test?
Storybook
npm installto download dependenciesnpm run storybook:devunit controlGutenberg
npm installto download dependenciesnpm run devTypes of changes
Checklist: