Fix lightbox UI disallow editing #59890
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
|
Size Change: +1.18 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
What should the behavior be in the event that an image block has an override of This can happen if a user sets a local override for the lightbox while using a In this PR, the block's local lightbox config would supersede the theme.json's default config, but with the UI hidden, it would also be impossible to reset the local value without the opening the code editor or otherwise modifying the block attributes manually. Is this the right approach? |
artemiomorales
added
[Type] Bug
artemiomorales
requested review from
ellatrix and
michalczaplinski
as code owners
|
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. |
This comment was marked as outdated.
This comment was marked as outdated.
gziolo
added
[Type] Regression
|
Ok this doesn't work as expected yet and mistakenly shows a blank popover if a link is configured and the lightbox's |
In the ideal world, we would have some kind of "log" and the latest setting would take priority (in your case, the value from the IMO, the least suprising behavior is if the local setting always overrides the |
|
I'd argue that preexisting block settings should take precedence, regardless of what the active theme supports. This would be consistent with other attributes. For example, if I set a custom text color for a Paragraph block with a theme that has custom colors enabled, that same text color persists if I switch to a theme where custom colors are disabled. I wouldn't break from the precedent set with other block settings, and I don't know of any other instances where this works differently. But that's a larger problem that I wouldn't worry about specifically for this ticket (don't want to get too far into the weeds). The goal should be to display the control based on the same conditionals present in WP 6.4. |
Ok got it @justintadlock @michalczaplinski. I went ahead and redid the implementation to allow the preexisting block settings to take precedence while gracefully handling any changes. There were various edge cases, and actually just decided to address all of them, updating the description with new testing instructions and a few walkthrough videos to go through the scenarios. Let me know what you think, thanks! |
|
Apologies, I didn't get to review it fully today. I ll finish up tomorrow. |
Comment on lines
+278
to
+280
| // When deleting a link from an image while lightbox settings | ||
| // are enabled by default, we should disable the lightbox, | ||
| // otherwise the resulting UX looks like a mistake. |
There was a problem hiding this comment.
I want to make sure that I understand correctly:
The case of image block with BOTH a link and a lightbox enabled should technically never happen. Or, at least it's not possible to get into this state using the UI. It might still happen if someone e.g. edits the attributes manually. So we have to handle this edge case somehow. Is that right?
There was a problem hiding this comment.
This one is actually a pretty common case! If at any time, a user has the following in their theme.json...
{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 2,
"settings": {
"blocks": {
"core/image": {
"lightbox": {
"allowEditing": true,
"enabled": true
}
}
}
}
}
...and also has a link configured for the image, then one potential way to handle that UX would be to show the popup for the link, and if that link is removed, then immediately show the popup for the lightbox. However, I found that UX to be confusing.
Here I go over more in depth what the issue is. Let me know what you think, and I'm also happy to discuss more or answer any questions 😄
lightbox-ui-exception3.mp4
There was a problem hiding this comment.
ok, I understand it now. Thank you for the explanation!
I agree that this is a better UX than just "jumping" to the lightbox. 👍
Could you add a link to this PR in this very comment in the code (line 280)? It might be helpful for someone in the future to refer to your video explanations.
michalczaplinski
approved these changes
Mar 20, 2024
There was a problem hiding this comment.
Awesome work, thank you for the exhaustive video explanation - it made reviewing much easier 🙂
The changes seem good to me 👍
If there is both a link and a lightbox enabled: true on the block we have to handle it in a slightly odd way (as I mention in this comment) but I think it's a very unlikely edge case and I can't think of a better way.
It would also be good to have some e2e test coverage for this logic but it's out of scope for this PR.
justintadlock
approved these changes
Mar 21, 2024
@michalczaplinski My plan is to start working on e2e tests this week to make sure all of these scenarios are covered 👍 |
Fix issue wherein lightbox popover was rendering erroneously when a link had been configured.
In some cases, such as when lightbox settings already exist for a block when global lightbox settings in theme.json change, we should allow users to see the lightbox UI and change the settings if they conflict with the global settings, even if the lightbox UI is disabled globally. This prevents a block from getting stuck with legacy lightbox settings and allows users to reset the block-level lightbox settings if need be in these edge cases. Note: We do not display the UI if the block-level settings exist and match the global settings, as the block will behave as expected in those circumstances and showing the UI in those circumstances would likely just be confusing.
artemiomorales
force-pushed
the
fix/lightbox-disallow-editing
branch
from
a8facb0 to
e1af101
Compare
This was referenced Mar 22, 2024
getdave
pushed a commit
that referenced
this pull request
Mar 25, 2024
* Check that lightbox can be edited before rendering lightbox UI * Refactor to reduce duplicate code * Fix and clarify component rendering logic Fix issue wherein lightbox popover was rendering erroneously when a link had been configured. * Reset lightbox attributes when removing link * Show lightbox UI if block-level override differs from default In some cases, such as when lightbox settings already exist for a block when global lightbox settings in theme.json change, we should allow users to see the lightbox UI and change the settings if they conflict with the global settings, even if the lightbox UI is disabled globally. This prevents a block from getting stuck with legacy lightbox settings and allows users to reset the block-level lightbox settings if need be in these edge cases. Note: We do not display the UI if the block-level settings exist and match the global settings, as the block will behave as expected in those circumstances and showing the UI in those circumstances would likely just be confusing. * Handle edge case of removing existing link when lightbox is fully enabled * Fix focus loss preventing end-to-end test from passing * Add link to PR in comment Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jeherve <jeherve@git.wordpress.org>
|
This PR was triaged by Editor release leads and contributors in WP Slack as part of the final WordPress 6.5 process. See https://wordpress.slack.com/archives/C02QB2JS7/p1711370484796489 (requires registration). The criteria used was:
Conclusion: this fix will be not be included in WP 6.5 but will be punted to WP 6.5.1. |
getdave
removed
the
Backport to WP 6.7 Beta/RC
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* Check that lightbox can be edited before rendering lightbox UI * Refactor to reduce duplicate code * Fix and clarify component rendering logic Fix issue wherein lightbox popover was rendering erroneously when a link had been configured. * Reset lightbox attributes when removing link * Show lightbox UI if block-level override differs from default In some cases, such as when lightbox settings already exist for a block when global lightbox settings in theme.json change, we should allow users to see the lightbox UI and change the settings if they conflict with the global settings, even if the lightbox UI is disabled globally. This prevents a block from getting stuck with legacy lightbox settings and allows users to reset the block-level lightbox settings if need be in these edge cases. Note: We do not display the UI if the block-level settings exist and match the global settings, as the block will behave as expected in those circumstances and showing the UI in those circumstances would likely just be confusing. * Handle edge case of removing existing link when lightbox is fully enabled * Fix focus loss preventing end-to-end test from passing * Add link to PR in comment Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jeherve <jeherve@git.wordpress.org>
youknowriad
added
the
Backport to WP Minor Release
tellthemachines
pushed a commit
that referenced
this pull request
Apr 9, 2024
* Check that lightbox can be edited before rendering lightbox UI * Refactor to reduce duplicate code * Fix and clarify component rendering logic Fix issue wherein lightbox popover was rendering erroneously when a link had been configured. * Reset lightbox attributes when removing link * Show lightbox UI if block-level override differs from default In some cases, such as when lightbox settings already exist for a block when global lightbox settings in theme.json change, we should allow users to see the lightbox UI and change the settings if they conflict with the global settings, even if the lightbox UI is disabled globally. This prevents a block from getting stuck with legacy lightbox settings and allows users to reset the block-level lightbox settings if need be in these edge cases. Note: We do not display the UI if the block-level settings exist and match the global settings, as the block will behave as expected in those circumstances and showing the UI in those circumstances would likely just be confusing. * Handle edge case of removing existing link when lightbox is fully enabled * Fix focus loss preventing end-to-end test from passing * Add link to PR in comment Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jeherve <jeherve@git.wordpress.org>
|
I just cherry-picked this PR to the add/fixes-for-651 branch to get it included in the next release: b54a426 |
tellthemachines
removed
the
Backport to WP Minor Release
tellthemachines
added a commit
that referenced
this pull request
Apr 9, 2024
* Font Library: Reset notices when navigating away from the collection (#59981) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: madhusudhand <madhudollu@git.wordpress.org> * Pattern Explorer: Pass 'rootClientId' to the pattern list (#60014) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: dsas <dsas@git.wordpress.org> * Fix lightbox UI disallow editing (#59890) * Check that lightbox can be edited before rendering lightbox UI * Refactor to reduce duplicate code * Fix and clarify component rendering logic Fix issue wherein lightbox popover was rendering erroneously when a link had been configured. * Reset lightbox attributes when removing link * Show lightbox UI if block-level override differs from default In some cases, such as when lightbox settings already exist for a block when global lightbox settings in theme.json change, we should allow users to see the lightbox UI and change the settings if they conflict with the global settings, even if the lightbox UI is disabled globally. This prevents a block from getting stuck with legacy lightbox settings and allows users to reset the block-level lightbox settings if need be in these edge cases. Note: We do not display the UI if the block-level settings exist and match the global settings, as the block will behave as expected in those circumstances and showing the UI in those circumstances would likely just be confusing. * Handle edge case of removing existing link when lightbox is fully enabled * Fix focus loss preventing end-to-end test from passing * Add link to PR in comment Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: jeherve <jeherve@git.wordpress.org> * Only show inserter in document tools if DFM is off (#60426) Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> * only show inserter in document tools if DFM is off * remove useless CSS hiding the inserter in DFM whcih is not rendered anymore * Fix don't close overlay menu when focus leaves submenu (#60406) Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> * Fix experimental useHasRecursion deprecation (#60451) Unlinked contributors: albanyacademy. Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> * Fix pattern block recursion handling (#60452) - Trigger recursion short circuit as early as possible before any other effects that can reason about inner blocks have run. - Use separate wrapper components to do this to satisfy the rule of hooks. Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> * remove alpha from edit post header (#60431) * Update the query block to permit non-core interactive blocks (#60006) * updated the query block to permit non-core interactive blocks * updated logic to correctly check all blocks inside the query support interactivity * removed check for core blocks * updated variable names and modal message per feedback * renamed variable blockSupportsInteractivityBool to blockSupportsInteractivity Unlinked contributors: poof86. Co-authored-by: colinduwe <colind@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: sethrubenstein <smrubenstein@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> * Add context to 'Library' string (#60520) Co-authored-by: ocean90 <ocean90@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> * DateTimePicker: Change day button size back from 32px to 28px (#59990) * DateTimePicker: Change day button size back from 32px to 28px * Update changelog Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> * Avoid overriding custom settings on font library save (#60438) Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: desrosj <desrosj@git.wordpress.org> Co-authored-by: estelaris <estelaris@git.wordpress.org> Co-authored-by: YanCol <collet@git.wordpress.org> --------- Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: madhusudhand <madhudollu@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: dsas <dsas@git.wordpress.org> Co-authored-by: Artemio Morales <artemio.morales@a8c.com> Co-authored-by: artemiomorales <artemiosans@git.wordpress.org> Co-authored-by: michalczaplinski <czapla@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: jeherve <jeherve@git.wordpress.org> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org> Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: colinduwe <colinduwe@gmail.com> Co-authored-by: colinduwe <colind@git.wordpress.org> Co-authored-by: sethrubenstein <smrubenstein@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: Dominik Schilling <dominikschilling+git@gmail.com> Co-authored-by: ocean90 <ocean90@git.wordpress.org> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: desrosj <desrosj@git.wordpress.org> Co-authored-by: estelaris <estelaris@git.wordpress.org> Co-authored-by: YanCol <collet@git.wordpress.org>
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 fixes an issue wherein the
allowEditingvalue of the lightbox was not resulting in a consistent UX. It also addresses other related edge cases.Why?
Fixes #59837
Fixes #59797
When one disables editing of the lightbox, the expectation is that the UI related to the lightbox config should be seamlessly removed from the editor.
How?
It adds logic to handle the case wherein the lightbox animation is enabled by default but editing of the lightbox is not.
It also covers various edge cases regarding how the block-level settings interact with the
theme.jsonsettings.Testing Instructions
To make sure this is working properly, We need to test various scenarios and edge cases.
In particular, we need to test the UI in the following scenarios:
In addition, each one of the scenarios above needs to be tested with the following
theme.jsonconfigurations:allowEditing false, enabled false
allowEditing true, enabled false
allowEditing false, enabled true
allowEditing true, enabled true
To test:
theme.jsonTesting overview
lightbox-settings-overview-edited.mp4
Test cases walkthrough
Part 1
I mispeak early in this video — in the first scenario, the lightbox UI should be disabled.
lightbox-settings-test-cases-pt-1.mp4
Part 2
lightbox-settings-test-cases-pt-2.mp4