Allow more columns in manual grid layout and change toggle order #59116
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
tellthemachines
added
[Type] Enhancement
|
Size Change: -1.53 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e1a08ec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7954650927
|
Wouldn't be against splitting |
Do we want there to be a max for the input? If we do that we'll either have to declare a |
|
Something else I'm wondering about is if the Range and Input should be on the same side in both Auto and Manual options; currently they're switched around: 
|
|
Ok in order to match both markup and appearance of the Auto option controls, I've changed to using the RangeControl with no input and a NumberControl wrapped in a fieldset. The input has no limit; the range is limited to 16. Marking this ready for review now! |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| </BaseControl.VisualLabel> | ||
| <Flex gap={ 4 }> | ||
| <FlexItem isBlock> | ||
| <NumberControl |
There was a problem hiding this comment.
When moving to a separate NumberControl and RangeControl component, I think both components will still need a label prop in order for the fields to be accessible. There was a PR a little while back for fixing this in the HeightControl component: #57683 which resolved the issue described in #57681. (TL;DR — it turned out the fieldset wasn't enough of a label for the controls, but we can use label and hideLabelFromVision props on these two components to get the labels linked). What do you think?
There was a problem hiding this comment.
Oh good point! We'll need the same in the min column width controls too; I'll just add it here.
andrewserong
approved these changes
Feb 19, 2024
There was a problem hiding this comment.
This is looking nice, and thanks for updating the labels! The consistency between the two modes feel really solid in the UI now that the components look similar too:
2024-02-19.13.40.35.mp4
One thing that I noticed while testing (not a blocker for this PR) is that if you go to edit the number of columns in Manual mode via keyboard, if you press backspace to clear out the field, it'll switch back to Auto mode before you have a chance to type a number. I don't think this is a huge deal as you can still change the number of columns with the up and down arrows, but I wondered if it might be worth only switching to Auto (or clearing out the value entirely) on blur instead of on change if the value is empty?
In any case, not a blocker for this PR if we want to look into it separately.
LGTM! ✨
Ah, well observed. This happens because the way we're currently distinguishing between Manual and Auto is through the existence of Another thing I'm noticing is that, unlike with the minimum column width controls, the range control for column count doesn't move when the value in the input changes. It's a bit weird because if we set a number in the input, even if it's within range, the range shows a completely different value. I'm not quite sure why, looking into it now. |
…'s peculiarities.
Turns out RangeControl is unable to process string values so I fixed this by making sure we always pass it a number 🤷 |
That's working beautifully for me! |
14 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?
Follow-up from #59051 (review).
Switches the order of Manual/Auto in grid layout sidebar controls and increases the RangeControl max columns to 16.
Todo: switch the input with a custom one so it can allow higher values (the default RangeControl input shares the max with the range control itself)
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast