Image block: Move UI for lightbox from sidebar to the content toolbar alongside link settings #57608
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
|
Size Change: +86 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
artemiomorales
changed the title
69 tasks
|
A perhaps premature review, here's a GIF showing the move of expand on click to the toolbar:
Looks great, works great, and will pair well with lightbox working best when it's on by default as set by the them and something you untoggle on a per image basis. There are some UX issues with the link UI toolbar, I suspect most of them are unrelated to this PR (dark border, the height, unifying the UI for linking and unlinking across the 3 types of link tokens, etc). Those shouldn't block this PR, but it'd be nice if some of the separators connected properly, and icons were aligned across. CC: @getdave and @richtabor for awareness. |
gziolo
added
[Block] Image
|
This new flow looks much more intuitive. Would it make sense to put an expand icon marker on the image to replicate better how it looks on the front end and to emphasize more this selection in the editor? |
Can make sense, though I would not make it a priority, and it can be entirely separate. |
|
@richtabor and I are working through Link UI issues. You can follow along at #50891 |
|
@artemiomorales @gziolo just checking in here, I'm super anxious and excited to get this PR in, and I'm eyeing a backport date of February 13th. Let me or anyone from @WordPress/gutenberg-design know if we can help with this one, it's quite important to get this flow right, so thanks so much for working on it 🙏 |
|
This is on my review list. If there's any UX/Design to review here then perhaps @richtabor would be well placed given he and I have been working on the Link UI together. |
|
I pushed some changes to get it closer. @artemiomorales, two follow-ups we need:
Ideally this should eventually leverage LinkControl, so there can be proper suggestions, and linking within WordPress is the same everywhere. I imagine these controls much like "Create page" is, additional at the foot of the suggestions:
|
@richtabor Great, thank you 🙏 |
|
Flaky tests detected in 13bea5a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7701649513
|
artemiomorales
force-pushed
the
add/lightbox-toolbar-ui
branch
from
899e1cf
to
2701608
Compare
artemiomorales
requested review from
ellatrix and
michalczaplinski
as code owners
@richtabor This is something to handle in a different PR, right? |
artemiomorales
commented
Jan 21, 2024
jasmussen
approved these changes
Jan 22, 2024
There was a problem hiding this comment.
This is a massive step forward. Thank you. Here's a quick GIF showing the control when lightbox is off by default in theme.json, with all of the controls move to the Link button:
Noting also that this works correctly when lightbox is enabled by default, it shows the link control linked to expand by default:
Simply based on how much more intuitive this behavior is, this PR is ready to land.
Rich gives some good suggestions for followups in this comment, I would not say those are blocking, and they can certainly be refined in the beta period.
To expand a bit, these are the main pieces of UI to polish, and I'll defer to Rich and Dave on how to best do that since it involves the link UI:
Some notes on those, most notably that the link dialog should sit 12px below the block toolbar, like other popovers:
Ideally we have helper text for image file and attachment options as well, so the three options are equally weighted.
And finally, if we can fidget the line-height of the linked state so the overall popover is exactly 48px tall to match the toolbar, that would be great. That's this one:
In general the inner padding, vertical alignment of icons and controls inside can be polished a bit to be balanced all around.
But as noted, not a blocker, and ideally some work I can help with. Thank you!
|
Two notes on the technical side. Unit tests fail on CI due to changes in WP core that require a follow-up bug fix. There are some e2e test failures reported, too. It looks like some UI labels have changed in this PR so that also needs to be reflected in the tests. |
Sure, let's include if possible @jeryj comment. |
|
It should always just say "Link", just like the standard link control as seen in the paragraph block. |
|
@richtabor So, the changes needed:
|
Changes added here. |
cbravobernal
pushed a commit
that referenced
this pull request
Jan 30, 2024
… alongside link settings (#57608) * Add first pass at Expand on Click in toolbar * Style changes * Fix lightbox setting check for toolbar * Remove old UI * Sync toolbar UI with global styles settings * Compact buttons * Update style.scss * Update UI * Make input width smaller * Remove chevron in Expand on Click popover * Improve markup and styles * Remove erroneous closing of popover after link is removed * Improve button label for removing lightbox * Update label name in test --------- Co-authored-by: Rich Tabor <hi@richtabor.com>
youknowriad
pushed a commit
that referenced
this pull request
Jan 31, 2024
… alongside link settings (#57608) * Add first pass at Expand on Click in toolbar * Style changes * Fix lightbox setting check for toolbar * Remove old UI * Sync toolbar UI with global styles settings * Compact buttons * Update style.scss * Update UI * Make input width smaller * Remove chevron in Expand on Click popover * Improve markup and styles * Remove erroneous closing of popover after link is removed * Improve button label for removing lightbox * Update label name in test --------- Co-authored-by: Rich Tabor <hi@richtabor.com>
|
This PR has been cherry-picked for |
cbravobernal
removed
the
Backport to Gutenberg RC
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @jeryj. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
This was referenced Feb 2, 2024
|
Another regression: #58599 |
This was referenced Feb 6, 2024
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?
This PR moves the UI for the image lightbox from the sidebar's block settings to the context toolbar so that it can exist alongside the rest of the link logic.
Why?
Addresses #54916
Right now the UX for the lightbox is divorced from the rest of the handling for links.
This consolidates it and makes it more intuitive.
How?
In the
image-url-input-uicomponent, this PR adds the logic, markup, and styles for the new design andExpand on clicktoolbar item. Also, inimage.js, I modified the logic for reading from or overriding the lightbox setting from the global styles.Testing Instructions
Post Editor
With global styles
expand on clicksetting enabledExpand on clickis enabledExpand on clickoption is activeExpand on click"lightbox":{"enabled":false}has been added in the block delimiterExpand on click"lightbox":{"enabled":false}has been removedWith global styles
expand on clicksetting disabled(same as above, but in reverse)
Expand on clickis disabledExpand on clickoption is deactivatedExpand on click"lightbox":{"enabled":true}has been added in the block delimiterExpand on click"lightbox":{"enabled":true}has been removedSite editor
For the site editor, please test the same as above. In addition, verify that toggling the global styles
Expand on ciicksetting also toggles the link UI for the image.Also, check that adding and removing
Link to image file,Link to attachment page, and custom links still works as expected.Testing Instructions for Keyboard
This uses existing UI and should work as expected with standard keyboard navigation.
Screenshots or screencast
lightbox-new-ui-test-trimmed.mp4