Move gallery link controls to the block toolbar #62762
Conversation
akasunil
added
[Type] Enhancement
|
Size Change: +952 B (+0.05%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
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: @randomburner, @RCNeil. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
…update/gallery-block-image-link-control
There was a problem hiding this comment.
Thanks for the PR!
I did a quick test and found some layout issues:
- When the Gallery block is inside a Group block, the parent selector button is small in size.
- The link buttons seem to be slightly wider on both sides.
I think the component to use here is ToolbarDropdownMenu.
If you search the packages/block-libraty directory for fToolbarDropdownMenu, you will find several implementation examples.
Additionally, the text of the following messages has become longer and could be made clearer.
trunk:
This PR:
|
|
I appreciate your feedback.
I updated the texts to reflect the suggestions made here on issue |
akasunil
requested review from
dmsnell,
tellthemachines,
spacedmonkey,
desrosj,
ellatrix,
dcalhoun,
ajitbohra,
peterwilsoncc,
gziolo,
ntwb and
nerrad
as code owners
|
@madhusudhand I am hesitant to add the new "Expand on clock" option to the Gallery block in this PR because there are a lot of things that need to be taken into consideration. See this comment. |
|
@akasunil I tested the new "Expand on click" at gallery against Gallery: Add lightbox support and it work perfectly fine. But it is best to bring the lightbox related changes into a new PR and keep the current PR specific to movement of the toolbar option, by doing so, it doesn't have to wait for #62906 |
I agree. |
|
Thank you @madhusudhand, I'll revert the change for |
| <MenuGroup className="blocks-gallery__link-to-control"> | ||
| { linkOptions.map( ( linkItem ) => { | ||
| const isOptionSelected = | ||
| linkTo === linkItem.value; | ||
| return ( | ||
| <MenuItem |
There was a problem hiding this comment.
@geriux @dcalhoun, If you know why the unit tests are failing, I would appreciate it if you could let me know 🙇♂️ This PR moves the link control of the Gallery block from the block sidebar to the block toolbar.
First, thank you for attempting to resolve the mobile unit test failures and for the ping. Much appreciated! 🙇🏻
Both MenuGroup and MenuItem are unavailable for the native mobile editor currently. This results in the crash and unit test failure regarding the Dropdown component attempting to render/return an undefined value.
We encountered this situation before and addressed it in #35191 by bifurcating the code along the web and native platform line. It is not an ideal solution — see #35191 (comment) — but, unfortunately, the lack of support for MenuGroup and MenuItem remains the current state.
If we take a similar approach here, we might retain the previous SelectControl for native platforms. I opened #63953 implementing this option, if we'd like to use this approach to move the proposed web changes forward quickly.
Unfortunately, I do not believe the native mobile team will prioritize MenuGroup or MenuItem components in the near future.
There was a problem hiding this comment.
@dcalhoun appreciate your response. Thank you for the details on the issue and PR too.
There was a problem hiding this comment.
@t-hamano is it worth doing this way?
|
Hi all, after taking a look at this discussion and related ones in #62906, I posted a summary with some potentially helpful context that can perhaps offer ideas on next steps and/or cases to consider 🙏 Thanks @akasunil for taking a look into this! |
The `MenuGroup` and `MenuItem` components are currently undefined for the native mobile editor. Therefore, we cannot render them in code shared between web and native without platform conditionals. Ideally, we add proper support for the native platform to avoid these conditionals, and their complexity and bundle size increase.
+one minor tooltip difference. Image calls it @jasmussen Can you suggest on the tooltip? |
That's good suggestion. |
There was a problem hiding this comment.
I appreciate your efforts in resolving the problem 🙇♂️
I think this PR is ready to ship, is it ok to merge this PR before moving forward with #56310?
There was a problem hiding this comment.
I think this PR is ready to ship, is it ok to merge this PR before moving forward with #56310?
@t-hamano Yup! I think merging this PR first or in parallel makes sense.
However, for me the issue mentioned here is still present, namely that the link icon in the gallery toolbar is not highlighted when a selection is made.
When an option for the gallery link is selected, the icon should appear highlighted, the same as happens with the icon in image toolbar. Here's a video demonstrating the contrast:
lightbox-link-icon.mp4
From my understanding, this is expected. Because this dropdown is just a tool to change the Indeed, the Gallery block itself has the For example, change the "Link To" value in the Gallery block to "Link images to media files". Then the user unlinks all the images in it. It seems strange that the link icon is highlighted in the block toolbar of the Gallery block, even though all the images don't have links or lightboxes. |
|
Shall we merge this PR? |
|
Before merging this PR, let's all agree and confirm that the issue mentioned in this comment is indeed the problem that this PR should solve or not. |
@t-hamano can you share your thoughts on this? |
This suggestion makes sense and I think it's OK to make this change in this PR. |
|
Flaky tests detected in 16f87e7. ��� Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10121224308
|
There was a problem hiding this comment.
Changes looks good and Let's merge this PR. 🚀
@akasunil A followup PR can be created for this issue (highlighting link icon) for design feedback and approval, based on which the change can be included or ignored.
What?
Fixes: #55022
Why?
Required to move gallery block's "Link to" control from the inspector to a more familiar, and consistent, link control in the block toolbar.
Screenshots or screencast
Edit-Post-.Hello-world-.-.-gutenberg-.-WordPress.1.webm