Adjust the custom range steps to match the units chosen #44959
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
the
[Feature] Design Tools
|
Size Change: +272 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
glendaviesnz
force-pushed
the
fix/spacing-custom-range-values
branch
from
8ad68cb
to
e0c71be
Compare
talldan
approved these changes
Oct 17, 2022
There was a problem hiding this comment.
Works well 👍
I noticed a couple of things, but they're only partially related to this change (and happen in trunk, so outside the scope of this PR).
- Switching between em/rem to px can be a way to 'hack' a floating px value. When manually typing in 10.1px, as soon as I unfocus it resets to 10px. OTOH if I first set 10.1em, and then switch from em to px, the value stays at 10.1px. Very low priority issue, but it's a little bit of an inconsistency around how values are handled.
- The slider still has the same 300 max value for em/rm as it does for px, which makes adjusting to a precise decimal value difficult. The increment is really around 0.5 because of the size and scale of the slider. Because the ems/rems have a more pronounced effect, this makes it a little hard to use. It's more noticeable for border controls though, because only a very small portion of the slider is actually usable for setting a resonable em/rem border.
aristath
approved these changes
Oct 17, 2022
| @@ -228,6 +228,9 @@ export default function SpacingInputControl( { | |||
| value={ customRangeValue } | |||
| min={ 0 } | |||
| max={ 300 } | |||
There was a problem hiding this comment.
Addressing thje comment by @talldan above, we should probably change the max value depending on the selectedUnit. This sets max to 300 if using px, and 10 for other units.
Suggested change
| max={ 300 } | |
| max={ | |
| 'px' === selectedUnit ? 300 : 10 | |
| } |
Other than that, the PR looks good so I'll go ahead and pre-approve it pending this tweak 👍
|
Thanks @talldan, @aristath - looking a bit closer it seems like we need 3 different max values, 300 for px, 100 for %, vh, vw and 10 for rem,em, with only rem and em needing 0.1 increments. I have adjusted it to match the above, let me know if you see any problems with this breakdown of the max values. |
talldan
approved these changes
Oct 18, 2022
Comment on lines
233
to
235
| maxCustomValues[ selectedUnit ] | ||
| ? maxCustomValues[ selectedUnit ] | ||
| : 10 |
There was a problem hiding this comment.
Could possibly change this to maxCustomValues[ selectedUnit ] ?? 10, but I also wonder why em and rem are excluded from maxCustomValues?
Comment on lines
237
to
241
| step={ | ||
| [ 'px', '%', 'vh', 'vw' ].includes( selectedUnit ) | ||
| ? 1 | ||
| : 0.1 | ||
| } |
There was a problem hiding this comment.
Might be good to do this the same way maxCustomValues works, extract it out to a variable.
| @@ -163,6 +163,8 @@ export default function SpacingInputControl( { | |||
| ! showCustomValueControl && | |||
| currentValueHint !== undefined; | |||
|
|
|||
| const maxCustomValues = { px: 300, '%': 100, vw: 100, vh: 100 }; | |||
There was a problem hiding this comment.
This could be moved to file scope and named MAX_CUSTOM_VALUES to avoid the little bit of overhead of redefining it on every render.
|
Thanks for the feedback @talldan. What do you think of this approach to tidy it all up? If you think this works it would probably make sense to extract |
|
Looks good 👍
I had that thought too. If there's somewhere that works well it could help with consistency across components. I would wait until the need arises though (i.e. if this same approach is used for border) |
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Aligns the spacing custom range control steps with the units chosen
Why?
Other custom ranges, like Border, use a
1step forpx, %and0.1for others, so Spacing sizing should work the same.How?
Checks selected unit and sets step accordingly.
Testing Instructions
%orpxis set that range control has steps of1, and0.1for other unitsScreenshots or screencast
Before:
steps-before.mp4
After:
steps-after.mp4