Image block: UI updates for the image lightbox (redo) #54509
Conversation
Simplified the `__experimentalUseGlobalBehaviors` function by removing the 'source' parameter. Now, the function directly uses 'userConfig' for getting raw data and 'mergedConfig' for variable value.
…n of updates to the lightbox UI
Previously, the test was checking whether schema properties array had exactly 10 elements We're now checking for exactly 9 elements instead.
Simplified the ImageSettingsPanel and ScreenBlock components. More specific changes: - Removed `name` and `settings` from the ImageSettingsPanel - Use `userSettings` instead of `settings` in the ImageSettingsPanel - Modified `onChangeLightbox` logic, it now takes a new setting instead of a boolean and directly passes to `onChange` - Updated the ScreenBlock component to account for this refactor
A new option has been added to the lightbox settings in the Theme JSON reference guide and Gutenberg class. This `showUI` option allows users to toggle whether the Lightbox UI is displayed in the block editor or not. Also, updated the JSON schema accordingly to reflect these changes in theme.json files.
I moved the logic to determine whether the lightbox should display or not to two render_block_data filters. One of these filters is inside of the index.php so that itc can exist in WP core, the other inside of blocks.php in order to offer legacy support for the Behaviors syntax in the Gutenberg plugin. Using the render_block_data instead of render_block allows us to store a 'lightboxEnabled' value on the block, which we can use to determine whether the lightbox should be rendered in these two separate locations relatively cleanly without needing to touch the markup. I added behaviors back to the valid top-level keys so that we can read it to offer legacy support. Lastly, I set the lightbox.enabled attribute to NULL by default so that we can determine whether the Behaviors syntax should override it or not.
…ock editor UI should inherit the value from `theme.json`. Likewise, if no value is set in the block attributes, the block editor UI should inherit the value from the Global Styles of the Image.
| @@ -0,0 +1,3 @@ | |||
| <!-- wp:image {"lightbox":{"enabled":true},"id":8,"sizeSlug":"large","linkDestination":"none"} --> | |||
There was a problem hiding this comment.
It looks like this test isn't covering the deprecated behaviors, it should rather be:
<!-- wp:image {"behaviors":{"lightbox":{"enabled": true, "animation": "fade"}},"id":8,"sizeSlug":"large","linkDestination":"none"} -->The file ending with .serialize.html should contain the migrated version like in this file.
It results in the following changes:
diff --git a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.html b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.html
index 9feeb10595..cdf5416f3b 100644
--- a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.html
+++ b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.html
@@ -1,3 +1,3 @@
-<!-- wp:image {"lightbox":{"enabled":true},"id":8,"sizeSlug":"large","linkDestination":"none"} -->
+<!-- wp:image {"behaviors":{"lightbox":{"enabled": true, "animation": "fade"}},"id":8,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="" class="wp-image-8"/></figure>
<!-- /wp:image -->
diff --git a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.json b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.json
index a32f031dd3..73a483d911 100644
--- a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.json
+++ b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.json
@@ -6,12 +6,12 @@
"url": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==",
"alt": "",
"caption": "",
- "lightbox": {
- "enabled": true
- },
"id": 8,
"sizeSlug": "large",
- "linkDestination": "none"
+ "linkDestination": "none",
+ "lightbox": {
+ "enabled": true
+ }
},
"innerBlocks": []
}
diff --git a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.parsed.json b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.parsed.json
index 0ca652ff77..a582b93cab 100644
--- a/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.parsed.json
+++ b/test/integration/fixtures/blocks/core__image__deprecated-v8-deprecate-behaviors-lightbox.parsed.json
@@ -2,8 +2,11 @@
{
"blockName": "core/image",
"attrs": {
- "lightbox": {
- "enabled": true
+ "behaviors": {
+ "lightbox": {
+ "enabled": true,
+ "animation": "fade"
+ }
},
"id": 8,
"sizeSlug": "large",Everything works as expected, but it's still worth updating the code.
There was a problem hiding this comment.
Good catch, I'll update the file.
| export default function ImageSettingsPanel( { | ||
| onChange, | ||
| userSettings, | ||
| settings, |
There was a problem hiding this comment.
Should these props be named value and inheritedValue instead of userSettings and settings. I understand that it's a bit different than the other components like TypographyPanel because this one updates the settings rather than the styles but there's a small precedent. The DimentionsPanel also updates the "layout" which is saved in "settings".
I like to think of the components in separation (regardless of where the value ends up saved). If a component has onChange ideally, it also has value. settings is used in general for things that configure the behavior of the component, not the thing that is changed by the component.
There was a problem hiding this comment.
If a component has onChange ideally, it also has value.
I gotta acknowledge that it's a convention in React, but at the same time, having a value prop in this situation erases the meaning of the original prop (passed from the parent).
For example:
When I look at this, I know it's dealing with the lightbox setting. If settings were renamed to inheritedValue I wouldn't know if it's a setting, a style, or something else. At least this is something that confused me when I looked at other panels initially 🙂
There was a problem hiding this comment.
I understand that it might confuse you but I think consistency is better than personal opinion in this case. I value your opinion but if we adopt something I think we should adopt in all components and not just one. Also this has been discussed previously here #37064
There was a problem hiding this comment.
I see! I don't feel very strongly about this, and indeed whether it's confusing or not is subjective/personal 🙂. I'll update it in a follow-up PR. Thanks Riad!
There was a problem hiding this comment.
I am testing image light box with WP 6.4-beta1 without Gutenberg plugin. The image light box doesn't work when I create gallery but it works with normal image block.
There was a problem hiding this comment.
@lwangdu Thanks for taking a look! I just tested in the WP 6.4-beta1 and it's working for me. Note that we don't have full gallery support, so you won't be able to toggle between the images while the lightbox is open, though the zooming should work as expected.
What happens when you click on a gallery image on the front end? Does the image expand?
| if ( ! isset( $lightbox_settings ) || 'none' !== $link_destination ) { | ||
| return $block_content; | ||
| if ( ! isset( $lightbox_settings ) ) { | ||
| $lightbox_settings = gutenberg_get_global_settings( array( 'lightbox' ), array( 'block_name' => 'core/image' ) ); |
There was a problem hiding this comment.
We can't use this function here because block PHP files are auto-generated in core from the package. Any gutenberg namespaced function is exclusive to the plugin so will error in core.
I thought we'd added a check for gutenberg namespaces in block-library? Cc @anton-vlasenko and @gziolo any idea how this might have slipped through?
There was a problem hiding this comment.
The check for function names at the moment covers only definitions. Preventing the usage of functions prefixed with gutenberg_ could be the next step. The challenge though is that we still need to allow them when wrapped with:
if ( defined( 'IS_GUTENBERG_PLUGIN' ) && IS_GUTENBERG_PLUGIN ) {}I know it's possible with ESLint for JavaScript, but I'm not sure about linting for PHP.
There was a problem hiding this comment.
Oooh I see, thanks for explaining!
There was a problem hiding this comment.
Thank you for the ping, @Mamaduka and @tellthemachines. I've created a new GitHub issue for this and hope to submit a PR soon: #54745
michalczaplinski
added
Needs Dev Note
|
Thanks for writing me back. Gallery image doesn't expand when I click on a
gallery image on the front end.
…On Thu, Sep 28, 2023 at 10:56 AM Artemio Morales ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
packages/block-editor/src/components/global-styles/image-settings-panel.js
<#54509 (comment)>
:
> +
+export function useHasImageSettingsPanel( name, settings, userSettings ) {
+ // Note: If lightbox userSettings exists, that means
+ // they were defined via the Global Styles UI and
+ // will NOT be a boolean value or contain the `allowEditing`
+ // property, so we should show the settings panel in those cases.
+ return (
+ ( name === 'core/image' && settings?.lightbox?.allowEditing ) ||
+ !! userSettings?.lightbox
+ );
+}
+
+export default function ImageSettingsPanel( {
+ onChange,
+ userSettings,
+ settings,
@iwangdu Thanks for taking a look! I just tested in the WP 6.4-beta1 and
it's working for me. Note that we don't have full gallery support, so you
won't be able to toggle between the images while the lightbox is open,
though the zooming should work as expected.
What happens when you click on a gallery image on the front end? Does the
image expand?
—
Reply to this email directly, view it on GitHub
<#54509 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF5N5BOM73VTDPVZKUSC3NDX4W24LANCNFSM6AAAAAA42DIDYM>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@lwangdu Ok got it! Would you be able to file a bug report? That way we can continue the discussion, see if other folks can reproduce as well and hopefully resolve the issue 😄 |
|
Sure, I can do no that no problem. Thanks
…On Thu, Sep 28, 2023 at 11:30 AM Artemio Morales ***@***.***> wrote:
@lwangdu <https://github.com/lwangdu> Ok got it! Would you be able to
file a bug report
<https://github.com/WordPress/gutenberg/issues/new?assignees=&labels=&projects=&template=Bug_report.yml>?
That way we can continue the discussion, see if other folks can reproduce
as well and hopefully resolve the issue 😄
—
Reply to this email directly, view it on GitHub
<#54509 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF5N5BMA2KM7SWEP6UXXIDDX4W65FANCNFSM6AAAAAA42DIDYM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@artemiomorales when you have a chance: I'm not a native English speaker but between |
@afercia Two options: If the latter, we should bring it up in the issue and discuss with other folks who were involved with that decision. What do you think? |
I will create a new issue. |
This is a redo of #54071, which was accidentally merged into its dependent branch #53851 before the dependent branch was merged to trunk.
What?
This PR updates the editor UI for the image lightbox in preparation for WP 6.4 and offers backwards compatibility for the
behaviorssyntax.Why?
Address #53851
The lightbox was originally part of a proposed feature set called behaviors; however, we have since decided that the lightbox will instead exist as a just property of the image block for now, and we need to clarify the UI for end users as a result.
This PR is based off of #53851, which removes the
behaviorssyntax. Both of these PRs should be merged at the same time to ensure that the lightbox continues to function as expected.How?
Settingspanel for the image block, where the lightbox is surfaced as a toggle in both the Global Styles and block settings. This design follows the discussion in Image block: Revise Lightbox UI to remove reference to Behaviors #53403 (comment).behaviorssyntax, so we've set up an inheritance scheme across two filter functions — one to be included in WP Core as part of theimageblock, the other to remain as part of the Gutenberg plugin, where the legacy support will reside.ToolPanel, which means that the lightbox can be reset to default in the global styles or block settings.How to enable
Via Global Styles
Open the global styles for the
imageblock, then locate theSettingspanel and enable theExpand on Clicktoggle.lightbox-via-global-styles.mp4
Via Block Settings in Full Site Editor
Open the full site editor, add an
imageblock, locate theSettingspanel under the block's settings, and enable theExpand on Clicktoggle.Lightbox.via.Block.Settings.in.Full.Site.Editing.mp4
Via Block Settings in Post Editor
Open the post editor, add an
imageblock, locate theSettingspanel under the block's settings, and enable theExpand on Clicktoggle.ligthbox-via-block-settings.mp4
Via
theme.jsonfileYou can enable the lightbox in the theme.json by using either of the following syntaxes under
settings:Syntax 1:
Syntax 2
Legacy Syntax
To test backwards support for the legacy syntax, please enable the lightbox using both the
theme.jsonand block attributes. Note that the old syntax allows for specifying theanimation, eitherzoomorfade. If neither is specified, the lightbox defaults tozoom.Via Theme.json
Via Block Attributes
Resetting Values
You can reset the user-defined
Expand on Clicksetting in the global styles or block settings using theSettingspanel menu.Lightbox.Reset.Settings.mp4
What to Test
Testing Instructions for Keyboard
No special instructions here — the UI should be accessible as it was created using standard Gutenberg components.