-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
RangeControls: Improve UI + UX #19916
Conversation
This is great! A few pieces of feedback to talk about and then happy to review.
|
Also test integration with Gutenberg
Also add comments for hooks and functions
03ac853
to
66c86e6
Compare
@karmatosed Haii!
Shadow removed. In doing so, I noticed that there was barely any visual feedback to indicate a focus state. I've adjusted it so that the circle has a blue ring (similar to
That To make the arrows bigger, we'd need to do quite a bit of custom UI/CSS work. In which case, I'd advise perhaps updating the base Hope that makes sense! |
@@ -0,0 +1,8 @@ | |||
// Keeping this className style rule as it is currently being | |||
// used OUTSIDE of the RangeControl component. |
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.
Bad :)
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.
I did a quick search and I noticed that it's only used in FontSizePicker.
Maybe we can just move the style there and rename the class to follow our guidelines components-font-size-picker__number
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.
Sounds good to me! I'll do that now
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.
LGTM 👍 Thanks for your hard work here.
Rename rangeControl related ID and classNames to match the structure of FontSizePicker
Aww. Does that mean we can't use this package? |
Just to clarify, Apache 2.0 is compatible with GPLv3, but not earlier versions of the GPL. WordPress is GPLv2+ (which means it could legally switch to GPLv3+ to use this library, but dropping GPLv2 compatibility is probably not a reasonable option here). If you really want to use this particular library, you could ask the library contributors (there are 12 of them) to consider relicensing or dual-licensing under a license compatible with GPLv2+. |
@ZebulanStanphill Thanks for your insight! I'm going to explore alternative solutions now |
Updates! I took a look at various solutions, and nothing seemed to feel right. I've opted to remove cssjanus, and wrote a tiny albeit naively simple and basic utility. cc'ing @youknowriad and @aduth for thoughts! (Sidenote: My Regex game is quite weak 😂 . Please feel free to improve the implementation!) It's definitely not as seamless as |
Looks like there's some valid e2e test failures. |
@youknowriad Ah. Looks like I need to update the E2E selectors for |
By updating the className for the input number field
Yay! E2E is fixed 😍 |
value: 0, | ||
label: '0', |
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.
If it's a common use case that the label would be the same as value, could we make it optional and fall back to a stringified value
?
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.
I'm not sure if it's common yet :).
I can see cases where you may want to render '10%'
instead
- Type: `Boolean` | ||
- Required: No | ||
- Platform: Web |
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.
Would be good to describe the default here, since it might be unexpected that the default is true
(that it's an opt-out, not an opt-in).
.components-range-control__number { | ||
.components-font-size-picker__number { | ||
display: inline-block; | ||
font-weight: 500; |
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.
Why?
currentInput: null, | ||
} ), | ||
] )( RangeControl ); | ||
export const RangeControlNext = compose( withInstanceId )( BaseRangeControl ); |
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.
We could have dropped compose
here rather easily, as it really only provides value when composing multiple:
export const RangeControlNext = compose( withInstanceId )( BaseRangeControl ); | |
export const RangeControlNext = withInstanceId( BaseRangeControl ); |
import RangeControl from '../'; | ||
import RangeControl from '../index'; |
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.
Why?
top: -4px; | ||
width: 1px; | ||
|
||
${markFill}; |
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.
I see it's not really documented anywhere (other than "when in doubt, space it out"), but: I would expect this should be formatted as:
${markFill}; | |
${ markFill }; |
@jsnajdr Is this something we should/can integrate into wp-prettier
?
Edit: Now I'm confused, because I see the expected spacing in another example below. Is this something that Prettier doesn't have opinions about?
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.
It's a template string with backticks, isn't it? Then Prettier should have an opinion here and should do the `${ spaced }`
formatting.
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.
@jsnajdr That's what I would expect as well, but it does not appear to be working correctly.
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.
It looks like a bug in wp-prettier
. If the template literal is a styled component, with the styled.span
tag, it's treated specially. Thanks for finding and reporting!
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.
I created a wp-prettier
fix in this commit: Automattic/wp-prettier@01fd780
A fix was needed for all the template literals that have a recognized format (CSS, GraphQL, HTML...) and where the string itself is being formatted.
I usually release such patches when a new upstream version comes out. In this case, I'll be waiting for prettier@1.19.2
.
|
||
const TOOLTIP_OFFSET_HEIGHT = 32; | ||
|
||
export default function SimpleTooltip( { |
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.
Can we create an issue to track this technical debt?
/** | ||
* An incredibly basic ltr -> rtl converter for style properties | ||
* | ||
* @param {Object} ltrStyles | ||
* @return {Object} Converted ltr -> rtl styles | ||
*/ | ||
const convertLtrToRtl = ( ltrStyles = {} ) => { | ||
const nextStyles = {}; | ||
|
||
for ( const key in ltrStyles ) { | ||
const value = ltrStyles[ key ]; | ||
let nextKey = key; | ||
if ( /left/gi.test( key ) ) { | ||
nextKey = [ key.replace( 'left', 'right' ) ]; | ||
} | ||
if ( /Left/gi.test( key ) ) { | ||
nextKey = [ key.replace( 'Left', 'Right' ) ]; | ||
} | ||
nextStyles[ nextKey ] = value; | ||
} | ||
|
||
return nextStyles; | ||
}; |
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.
Seems like a thing we'd want unit tests for.
I see at least one possible issue: It won't handle anything short-hand that's still relevant for RTL, e.g.
margin: 0 10px 0 0;
Also, I'm confused about the reassignment of nextKey
. It's being replaced into an array? But then the array is being used as the key of an object? An array is not a valid key of an object.
At which point: The RegExp#test
s are redundant, because String#replace
will do nothing if the pattern is not found.
Also: String#replace
only replaces the first instance when provided a string, not all instances. You'd want your regular expression /left/gi
for that (key.replace( /left/gi, 'right'
).
Description
This update improves the UI and UX for the
RangeControl
component with@wordpress/components
.Features ✨
input[type="number"]
Example of step marks + Tooltip rendering:
A11Y Considerations 💪
input[range]
under the hood, which gives us native keyboard interaction handlingaria
roles on elements to emphasize changes / reduce noise (viaaria-hidden
)Other Considerations ✌️
How has this been tested?
This update was developed within Storybook. When ready, it was integrated and tested with Gutenberg, local dev.
Voiceover
This component was tested with Mac OS Voiceover, to ensure the value change announcements + keyboard interactions worked as expected.
Mobile
Although, this component only updates the Web version, the changes were tested on both iOS (iPhone 8) and Android (Samsung A50)
iPhone 8 Screen Recording
Note: The
inputMode
for the number input was set todecimal
, which allows for thenumpad
keyboard to appear for mobile.Types of changes
CSS
The CSS for the updated component was written with Emotion, leveraging the new/experimental CSS-in-JS system.
Tooltip
The Tooltip that appears to indicate the value is not a
@wordpress/components
one, but rather, a simple/custom implementation.Reason
I had originally tried using the Popover/Tooltip from
@wordpress/components
, but this led to (perceived) performance rendering issues:https://d.pr/i/a6i3R4
The positioning render cycle was not in sync with the quick changes, which made the experience look and feel really laggy.
As a work-around, I created a simple Tooltip which is currently only being used in the new
RangeControl
component. (I'm open to changing this approach if anyone has suggestions)Backwards Compatibility
This update preserves all of the Component APIs. Therefore, it shouldn't break backwards compatibility. It passes all of the previous unit tests and appears to be working as expected when testing within Gutenberg. Component API changes are additive.
Checklist:
Resolves: #19845