Image block: Revise lightbox UI to remove 'behaviors' #53851
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
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-rest-global-styles-controller-gutenberg.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ lib/compat/wordpress-6.4/class-gutenberg-rest-global-styles-revisions-controller-6-4.php ❔ lib/load.php ❔ lib/theme.json ❔ phpunit/class-gutenberg-rest-global-styles-revisions-controller-test.php ❔ phpunit/class-wp-rest-global-styles-controller-gutenberg-test.php |
|
Size Change: +91.9 kB (+6%) 🔍 Total Size: 1.62 MB
ℹ️ View Unchanged
|
Simplified the `__experimentalUseGlobalBehaviors` function by removing the 'source' parameter. Now, the function directly uses 'userConfig' for getting raw data and 'mergedConfig' for variable value.
|
I've pushed some last changes. There are a couple of things that are not working and we need final decision on how to structure the options. I've explained the changes here: 
|
…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.
| @@ -1117,108 +1019,6 @@ test.describe( 'Image - interactivity', () => { | |||
| ).not.toBeInViewport(); | |||
| } ); | |||
|
|
|||
| test.describe( 'Animation Select visibility', () => { | |||
There was a problem hiding this comment.
Removing this test as well, because there is no "animation selector" anymore. The Lightbox only has the "zoom" effect (animation) - it's the only one.
michalczaplinski
requested review from
spacedmonkey,
kevin940726,
Mamaduka,
ellatrix,
ajitbohra,
ajlende,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
michalczaplinski
added
the
[Feature] Interactivity API
Comment on lines
-3
to
-12
| "behaviors": { | ||
| "blocks": { | ||
| "core/image": { | ||
| "lightbox": { | ||
| "enabled": false, | ||
| "animation": "" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Can this just be removed or does it need a deprecation of some sort? Behaviors were shipped in GB 16.4 so never included in a WP release.
There was a problem hiding this comment.
@artemiomorales and I looked at whether this is used in production, and it looks like you can change that setting in the UI, which most likely gets reflected in what gets saved in the database. While it isn't mandatory for WordPress core, it would still be nice to provide a migration path for sites that have the latest Gutenberg plugin enabled. It shouldn't be that difficult to code something in the plugin to read behaviors object from the site and transfer the value to the new structure.
I guess the same applies to the block attribute that changes from behaviors.lightbox to lightbox in the Image block.
There was a problem hiding this comment.
I'm not fully caught up on what's next for behaviors, but it seems that themes will no longer be able to tweak the lightbox feature (enable/disable, set animation) after this removal.
If we still plan to allow them to tweak it, just in a different place, we could add some code around this line to make the migration easier for them. I'll be AFK in the next few days, but wanted to share in case it's useful.
There was a problem hiding this comment.
In the followup PR: #54071 we've added backwards-compatibility for users that might have been using the previous behaviors.lightbox syntax.
In that PR (which is based on the current one) we're also now introducing a lightbox.allowEditing setting which allows themes to enable/disable the lightbox.
69 tasks
86 tasks
|
I've taken this PR for a spin, and checked the global styles/revisions rest API changes and associated tests. From that perspective, it looks good to me. On the backwards compatibility subject - I've heard it more than once that the plugin itself supports the previous two versions only, when those changes do not impact on Core. Here's that noted in a comment: #47475 (comment) But I can't yet find anything official. I think it's because there is nothing official. Was this published as an experimental API? The use of I guess we should be always wary when it's a public API. It was a topic discussed at the WordPress community summit this year. See: https://make.wordpress.org/summit/2023/08/23/community-summit-discussion-notes-addressing-backwards-compatibility-in-gutenberg/ Related issue: |
|
Thanks a lot for the review, Ramon!
It was never shipped in Core, though, so I guess deprecation is the way to go! Just to let you know, there is a follow-up PR to this one: #54071 |
Behaviors are deprecated for 2 more releases and will be removed then.
|
Flaky tests detected in 91b86ef. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6145160023
|
…json." This reverts commit cabe31b.
19 tasks
|
I tested with the following block content and theme.json modification. <!-- wp:heading -->
<h2 class="wp-block-heading">Global</h2>
<!-- /wp:heading -->
<!-- wp:image {"id":68,"sizeSlug":"large","linkDestination":"none"} -->
<figure class="wp-block-image size-large"><img src="http://wp.local/wp-content/uploads/2023/02/27982606245_0a702a9b26_o-1024x576.jpg" alt="" class="wp-image-68"/></figure>
<!-- /wp:image -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Block</h2>
<!-- /wp:heading -->
<!-- wp:image {"id":68,"sizeSlug":"large","linkDestination":"none","behaviors":{"lightbox":{"enabled":true,"animation":"fade"}}} -->
<figure class="wp-block-image size-large"><img src="http://wp.local/wp-content/uploads/2023/02/27982606245_0a702a9b26_o-1024x576.jpg" alt="" class="wp-image-68"/></figure>
<!-- /wp:image -->{
// ...
"behaviors": {
"blocks": {
"core/image": {
"lightbox": {
"enabled": true
}
}
}
},
// ...
}This is what I see.
@artemiomorales The lightbox support wasn't hidden behind an experimental flag or anything in the UI, so folks using the plugin who have been using lightbox will see this as breaking their site if you don't have the migration. So I would recommend getting the migration handled before merging this PR so we can be sure they make it into the same GB release.
As this PR is right now, there is no backwards compatibility. The functionality has simply been removed. We have systems for migrating both block attributes and global styles settings that should be used. For block attributes, you just need to migrate the attributes to their new location with a block deprecation. Since the block supports code is removed, you'll have to replace the supports in the block deprecations with the For global styles (theme.json), we also have a migration system in class-wp-theme-json-schema.php. You can write a migration function to move the setting where you wish. The block supports part is the only part where you'll need to properly add a deprecation. That can be done simply by adding a Any public function you want to remove can't be deleted if you're providing backwards compatibility. Instead, you add one of our deprecated functions that will print a warning. Then in two versions, the function can finally be removed. |
* Add initial implementation of image settings panel
* Remove unnecessary code and fix reset functionality
* Add UI to image block inspector
* Add `lightbox` to the valid `theme.json` settings
* Fix a bug with image selector and integrate with global styles
* Update `theme.json` schema
* Added the `@since`annotation
* Refactor image settings panel and screen block
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
* Add showUI option to lightbox settings
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.
* Add defaults for the `lightbox` to the GB `theme.json`
* Change the falsy checks in image.js
* Add filters; add legacy support for behaviors syntax
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.
* Fix linter errors & add more expansive comments.
* If no value is set for the lightbox in the Global Styles, then the block 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.
* Rename `showUI` to `allowEditing`
* Fix the `theme.json` schema
* Use the globalPath for the settings on the PHP side
* Add backwards support for enabling fade animation via the legacy syntax
* Fix PHP linter errors
* Fix error when checking lightbox['enabled'] value in global settings
* Empty commit
* Remove the default `false` value for `lightbox.enabled`attribute.
* Add deprecation notice for 'behaviors' in image block
I needed to add 'behaviors' back to the block.json attributes
in order to read them on the JavaScript side in the editor
to fire the deprecation notice.
* Add deprecation for attribute in image block
* Remove obsolete code now that block deprecation is in place
* Add support for 'lightbox: true' syntax
* Fix lightbox 'checked' attribute being read improperly
* Add conditional display for settings panel at image block level
* Fix an error with the theme.json schema.
* Update docs with `npm run build:docs`
* Copy `class-wp-theme-json-schema.php` from core
into `class-wp-theme-json-schema-gutenberg.php`
* Add a theme.json migration to v3 away from behaviors and to a new, simpler syntax used by the lightbox.
* Remove the `null` value from lightbox.enabled in `lib/theme.json`
* Remove `behaviors` from VALID_TOP_LEVEL_KEYS & VALID_SETTINGS
* Revise backwards compatibility for behaviors; add deprecation to block_supports
* Remove outdated comment
* Update outdated comment
* Update comment and shuffle the lines so the diff is easier on the eyes.
* Update comment to explain why we use userSettings in image-settings-panel.js
* Remove support for legacy fade configuration
* Resolve lint error
* Add clarifying comment regarding lightbox markup
* Rename the migrate function to reflect that it's not a v3 migration
* Add a `@since` in `gutenberg_should_render_lightbox` docblock
* Add support for reading top-level 'lightbox' setting in editor
By default, we read the lightbox settings underneath the 'core/image'
in theme.json; however, the 'enabled' property there is undefined by default,
which means it should be possible to declare a top-level setting for the lightbox
that overrides an undefined block-level setting.
While this appeared to be working on the PHP side, the UI wasn't accurately
reflecting this inheritance structure, so this commit fixes that.
Users should now be able to define a top-level lightbox setting as either
'lightbox: true' or 'lightbox: { enabled: true }' that will be used
if the block-level lightbox setting for 'enabled' is undefined.
* Revert "Add support for reading top-level 'lightbox' setting in editor"
This reverts commit 2f5f122.
* Add correct deprecation mentioning the Gutenberg version
* Move 'allowEditing' to top-level settings
* Fix top-level lightbox setting not being read properly
* Fix 'false' values in theme.json not being stored in settings object
* Fix error wherein 'undefined' was being passed to input component
* Fix bug wherein lightbox UI would disappear if user value was set
* Remove inheritance when determining whether to enable lightbox
Rather than trying to check if the 'enabled' has been set or not
and falling back to other levels of the theme.json inheritance
structure, I decided to just read and use the settings as defined
in theme.json. This is the expected behavior in Gutenberg from
what I understand and has less edge cases.
* Update comment
* Update whitespace in theme.json
Co-authored-by: Alex Lende <alex+github.com@lende.xyz>
* Add docblocks to clarify that `class-wp-theme-json-schema-gutenberg.php` is only put in GB as a temporary migration.
* Add comments to clarify that behaviors.php is temporarily added to GB an will be removed in a future version.
* Added integration fixtures for the lightbox
* Fix incorrect reading of global lightbox settings
* Clarify the comment getting the global lightbox settings
* Fix PHPCS parenthesis error 🤦♂️
* Move lightbox settings to the block level
* Remove support for shorthand
* Remove 'lightbox' from hooks.js and add `.enabled` & `.allowEditing` to class-wp-theme-json-gutenberg.php
---------
Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com>
Co-authored-by: Alex Lende <alex+github.com@lende.xyz>
This reverts commit 1b3a480.
|
The backwards compatibility concerns have been addressed in #54509 👍 |
This was referenced Sep 18, 2023
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?
Now that the concept of behaviors is no longer shipping in the near term, this PR removes it from the
theme.json, the Global Styles and the Image block attributes.Why?
We want to provide a lightbox feature and get feedback on its implementation, but we also want to do so in a way that's minimal and easy to understand. Addresses #53403
How?
Removing all the references to
behaviors.Note regarding backwards compatibility: We've added backwards compatibility in the following PR, which is meant to be merged at the same time as this one so there's no gap in support for users using the old syntax we're removing in this PR.
Testing Instructions