Organize gallery controls #58407
Merged
Organize gallery controls #58407
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
richtabor
added
[Type] Enhancement
|
Size Change: +19 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in def7637. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7701656377
|
|
Another order might be:
This way resolution and crop are grouped which are semi related, and link to and open in tab are grouped which are related but also prevent layout shift. |
Yea, I thought about that too, but I was trying to keep the toggle controls together, to maintain a sense of visual hierarchy. And I don't like the "Open new tab" control jumping in the middle of the mix. |
|
Personally, I like this order in PR. One thing that bothers me is that the explanation for "Crop images" is omitted. Does it make sense to change the toggle text itself to "Crop image to fill entire column" like in the Media & Text block?
|
|
Looks good, I also think i practice, grouping the toggles makes sense, but it's not a strong opinion. Aki makes a good point, I wonder if we can change it to "Crop images to fit"? |
jameskoster
reviewed
Jan 30, 2024
I omitted it because the description was not adding any value, just more words.
I like it! |
|
I made the labels a bit clearer based on suggestions:
|
jasmussen
approved these changes
Jan 31, 2024
t-hamano
approved these changes
Feb 2, 2024
There was a problem hiding this comment.
Let's merge this PR. UX improvements such as those discussed in this comment can still be discussed in #58558 as a follow-up.
The "Random order" checkbox itself has existed in the Gallery feature of WP Core for some time. And that functionality has already been carried over to the Gallery block using the exact same text. Changing the text from "Random order" to "Randomize order" is at least an improvement.
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf 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. |
|
I fixed the unit test and merged trunk into this PR again. I think all GitHub actions will be ✅. |
@t-hamano I noticed the React Native E2E test is failing; is that related here? |
Based on the failed tests, it doesn't appear to be relevant to this PR. Now that it has passed the test, I think it's safe to merge |
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?
Following up on #57477 by renaming and ordering the controls to better support visual and logical control hierarchy. Moves "Resolution" above "Link to" as its a higher priority control. And moves "Open images in new tab" to append to the controls, rather than displace.
How?
Testing Instructions
Screenshots or screencast