Behaviors UI #49972
Merged
Behaviors UI #49972
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
gziolo
added
the
[Feature] Interactivity API
gziolo
reviewed
Apr 21, 2023
gziolo
added
[Type] Feature
ajlende
reviewed
Apr 30, 2023
|
Size Change: +1.46 kB (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
michalczaplinski
commented
May 5, 2023
|
I've created a UI to enable the Behaviors in the "Advanced" panel of the block InspectorControls. It's still still WIP and completely broken at the moment, but it's a skeleton to build off. I'm using the gutenberg/packages/block-editor/src/hooks/behaviors.js Lines 66 to 70 in 5afa6d3 Just using |
|
Flaky tests detected in 9124b58. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5072350404
|
22 tasks
Should we use the behaviors inside block supports? Imagine the block.json file of image. "supports": {
"__experimentalBorder": {
"color": true,
"radius": true,
"width": true,
"__experimentalSkipSerialization": true,
"__experimentalDefaultControls": {
"color": true,
"radius": true,
"width": true
}
},
"__experimentalBehaviors": {
"lightbox": false
}
},That way we could conditionally check if the block supports the behaviors, which ones are supported and show the UI so the user can select which ones to apply. Something like this: // Check if the block has behaviors.
const blockBehaviors = select( blocksStore ).getBlockSupport(
props.name,
'__experimentalBehaviors'
); |
I haven't considered that, but it might make sense! This way, individual blocks could enable/disable the Behaviors UI using the I've added the outline of the technical details of the implementation (previously created by @SantosGuillamot), which briefly mentions the block supports in #50029 (comment) . Let's discuss this over there in the issue! |
|
I added a couple e2e tests. Feel free to update them if needed! |
|
The tests look great but I have some trouble running them locally @c4rl0sbr4v0 ! I've tried changing that to but that does not help either. Can you run them locally? |
|
I've pushed some more changes and I believe the implementation is close to what we want. There are still a few things left:
|
cbravobernal
force-pushed
the
behaviors-ui
branch
from
7f71175
to
4f9fd0b
Compare
|
I updated the branch with trunk and did
|
|
Hey @michalczaplinski, it's great to see this coming along! I looked at your video on the Outline for Block Behavior issue explaining your ideas on the UX and have two questions:
I'm wondering what would be the best way to ship this out to users. What we have right now will make the lightbox available to folks, but if our intent is to generate ideas and discussion around behaviors, maybe we can consider adding these two points. |
Yes! I run
I've created the theme like the style-variations you mention. Let's see if it also fails in CI. |
youknowriad
reviewed
May 24, 2023
| Object.entries( settings ).every( ( [ , value ] ) => ! value ) | ||
| ) { | ||
| return <BlockEdit { ...props } />; | ||
| } |
There was a problem hiding this comment.
There's a subtile issue that may or may not result in a bug. The fact that we conditionally render BlockEdit can result in a "remount" of the whole component if the condition above changes from "true" to "false" or the opposite.
The remount often create focus loss and issues like that. While this might not always be a problem, it is a problem in general if the condition "can" change when the component is mounted (I'm not sure it's the case here)
There was a problem hiding this comment.
That conditional is reading a theme.json value. I don't think that condition can change without a page reload 🤔
There was a problem hiding this comment.
Yeah, probably fine for now, just something to know as sometimes we used to switch over attribute values (easy to add a check there). Also, maybe at some point in the future, that setting could be editable in the global styles UI in the site editor.
There was a problem hiding this comment.
that setting could be editable in the global styles UI in the site editor
That is the next step after merging this PR, so we should then take a look at this issue.
cbravobernal
approved these changes
May 24, 2023
SantosGuillamot
approved these changes
May 24, 2023
There was a problem hiding this comment.
I've been testing the UX and it looks great 👏 Some comments:
Testing
I used:
settings.blocks.core/image.behaviors.lightbox: To enable/disable the Editor UI to select behaviors per block.behaviors.blocks.core/image.lightbox: To define a default value and activate/deactivate the lightbox in the images.
What I tested:
- In the theme
theme.json: NOsettings.blocks.core/image.behaviors.lightboxand NObehaviors.blocks.core/image.lightbox
It gets the default values from Gutenberg and it shows the UI to select the behaviors and the default value is "No behaviors".
- In the theme
theme.json: Addsettings.blocks.core/image.behaviors.lightbox = false
The UI to select the behaviors doesn't appear anymore.
- In the theme
theme.json: Addbehaviors.blocks.core/image.lightbox = trueand remove thesettings.blocks.core/image.behaviors.lightbox
The UI appears and the default value is "Lightbox". It works in the frontend.
- In the theme
theme.json: Addsettings.blocks.core/image.behaviors.lightbox = falseandbehaviors.blocks.core/image.lightbox = true
The UI to select the behaviors doesn't appear in the Block Editor BUT the lightbox is still working in the frontend.
- Overwrite the value for each block.
If I select a behavior from the block, it overwrites the default value defined by the theme or Gutenberg.
Potential improvements
I just have a few aspects to consider:
- I think we might need a "Reset" button to remove the lightbox from the block attributes and listen to the default value from the theme/Gutenberg. At this moment, if I select "Lightbox" from the Editor UI, there is no way to go back to the default value. I can change the attribute to "No behaviors", but it is not the same as listening to the default. This is something many styles are using for example.
- We might need to add the behaviors property to the schemas.
- I didn't test it, but I guess that, in the future when there are more behaviors, if
settings.blocks.core/image.behaviors.lightboxis false, it shouldn't appear in the dropdown list but it shouldn't remove the whole list of behaviors.
This reverts commit 70e51cd.
kevin940726
reviewed
May 24, 2023
Comment on lines
+12
to
+19
| const filename = '1024x768_e2e_test_image_size.jpeg'; | ||
| const filepath = path.join( './test/e2e/assets', filename ); | ||
|
|
||
| const createMedia = async ( { admin, requestUtils } ) => { | ||
| await admin.createNewPost(); | ||
| const media = await requestUtils.uploadMedia( filepath ); | ||
| return media; | ||
| }; |
There was a problem hiding this comment.
It's a convention to use the POM-style class as shown in other test file to create file-specific utilities. See the image.spec.js test for an example. It's not strictly required though, it's just a suggestion :)!
| }, | ||
| } ); | ||
|
|
||
| await page.getByRole( 'button', { name: 'Advanced' } ).click(); |
There was a problem hiding this comment.
Nit: We can scope it under the Editor settings sidebar to make it more readable. (It isn't clear which "Advanced" button we're clicking as it's a pretty generic name.)
Suggested change
| await page.getByRole( 'button', { name: 'Advanced' } ).click(); | |
| await page | |
| .getByRole( 'region', { name: 'Editor settings' } ) | |
| .getByRole( 'button', { name: 'Advanced' } ) | |
| .click(); |
| } ); | ||
|
|
||
| await page.getByRole( 'button', { name: 'Advanced' } ).click(); | ||
| const select = page.getByLabel( 'Behavior' ); |
There was a problem hiding this comment.
Try getByRole with parent locator here too?
Suggested change
| const select = page.getByLabel( 'Behavior' ); | |
| const select = editorSettings.getByRole( 'combobox', { name: 'Behavior' } ); |
| const select = page.getByLabel( 'Behavior' ); | ||
|
|
||
| // By default, no behaviors should be selected. | ||
| await expect( select ).toHaveCount( 1 ); |
There was a problem hiding this comment.
This isn't needed as a locator is strict by default.
| await expect( select ).toHaveValue( '' ); | ||
|
|
||
| // By default, you should be able to select the Lightbox behavior. | ||
| const options = select.locator( 'option' ); |
There was a problem hiding this comment.
Ditto. Try getByRole instead?
Suggested change
| const options = select.locator( 'option' ); | |
| const options = select.getByRole( 'option' ); |
| await page.getByRole( 'button', { name: 'Advanced' } ).click(); | ||
|
|
||
| // No behaviors dropdown should be present. | ||
| await expect( page.getByLabel( 'Behavior' ) ).toHaveCount( 0 ); |
There was a problem hiding this comment.
A more readable and explicit assertion might be:
Suggested change
| await expect( page.getByLabel( 'Behavior' ) ).toHaveCount( 0 ); | |
| await expect( locator ).toBeHidden(); |
|
@SantosGuillamot Thank you for testing thorougly 🙏
Agreed on all 3 points. I ll work on this in a follow up PR. |
cbravobernal
removed
the
[Status] In Progress
|
@kevin940726 Thanks for your comments! @Mamaduka has implemented the improvements you suggested, and I've just merged them 🙂 . |
ramonjd
added
the
Backport to WP Beta/RC
|
I've filed the backport for WP 6.3 here: WordPress/wordpress-develop#4526 |
ramonjd
removed
the
Backport to WP Beta/RC
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?
Adds a simple UI element to add specific behaviors to a block and support for them to
theme.json&block.json. It is only concerned with the built-in "LIGHTBOX" behavior that is worked on in #49621.The behavior can be defined in the following way:
Why?
We need a way for users to add specific Behaviors to their blocks. We also need them to be able to opt into them in the
theme.jsonfile.How?
https://github.com/WordPress/gutenberg/blob/77288e42e762c30f3599751c1810339e8aca7f69/packages/block-editor/src/hooks/behaviors.js#L83-L88
theme.jsonfile so that the core Image block uses the LIGHTBOX behavior by default.Testing Instructions
Screenshots
Screen.Recording.2023-05-09.at.20.52.10.mov