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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Image block: UI updates for the image lightbox #54071
Image block: UI updates for the image lightbox #54071
Changes from 27 commits
e613a5b
b0fb3b5
b76d56c
1872f77
3cfcb15
d2818af
a32d0f6
e67634e
3ddb1b1
fae6f99
df9f857
df3f1c6
075535b
c2a0b3f
91d130c
c58612c
c602139
339a715
4b43b30
ce3c338
7fefb83
03e60d7
050c636
521d950
1db73c5
715e32c
41f251d
79dad34
871ba45
7f2f70f
6da3545
83bef30
52e358d
11e96e2
8809612
59e0dbe
3eeb1a7
443aa5b
6459d56
203649d
5a37499
0e63890
8b721a7
8287c46
a2c2b94
4f0709b
7018ee7
7fd0801
5bb05bc
a02910c
b12d386
2f5f122
a238d94
0aba3c4
8fa991e
65b173c
b5f8186
7e121da
fb10384
ef8f43e
fc8fc47
6ea39f8
aaab1ef
61bfb3d
c18e283
7d30aa6
dab7f5a
a5c2f0c
485c197
d1432bc
ff1d2b5
ee1dd8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be more accurate?
If so, you could get rid of the second
useGlobalSetting()
in screen-block.js.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my first thougth as well, but it wouldn't work as expected!
The
hasValue
prop of theToolsItemPanel
controls whether the "RESET" button is enabled or not. We only want to enable it if there is a value in theuserSettings
, meaning that the user has set a value themselves in the Global Styles.E.g.: If the default value is
true
, but the user hasn't set anything in the Global Styles the checkbox should be checked, but it shouldn't be possible to "RESET" it:I agree that it's not too intuitive though so I ll add a comment explaining the above 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the code comment in 4f0709b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was testing things here, I found a bug related to this. It would be fixed by comparing the value with the theme.json origin default value instead of checking the user origin, but it's pretty minor, so I'm happy to leave this as a follow-up so we can get this merged before the feature freeze.
wp_global_styles
CPT either by deleting the one for the theme you're using in your database or using a new theme.reset-layout-lightbox-settings.mp4
What happens when you try to change anything in the settings part of theme.json, the entire settings object is saved in the
wp_global_styles
CPT. In the code here, you're checking for the value from this saved object to be truthy, and because all defaults are written to the database, not just the ones changed, setting any of them will trigger all of them to have their default value in the user settings.I see this as a side-effect of trying to store things that aren't Gutenberg UI settings (
layout.contentWidth
,layout.wideWidth
, andlightbox.enabled
) in the settings object. So I think it would be better to move these values out of the settings rather than updating the code to be more specific about which settings are saved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useMemo
is actually adding a lot of extra overhead here. It's just a boolean.While the above matches your logic exactly, this conditional has some unexpected results in my opinion. My examples use the theme.json settings, but relate to the attribute as well.
Here's one example that I think is the flip side of what @youknowriad was saying in https://github.com/WordPress/gutenberg/pull/54071/files#r1317152387.
<!-- { "lightbox": true } -->
Unexpectedly, this does not enable the lightbox.
We often follow this sort of pattern for settings related things in Gutenberg:
{ "appearanceTools": true }
.true
(as above).These rules don't always hold—we have a lot of inconsistencies. However, this set of rules allows for you to only have to write the minimal set of things they want to override.
It can be enabled with default options:
<!-- { "lightbox": true } -->
It can be enabled with custom options:
<!-- { "lightbox": { "animation": "fade" } } -->
It can be disabled when enabled by default in theme.json:
<!-- { "lightbox": false } -->
It can use the default in theme.json:
Moving on to theme.json settings. This example confuses me:
If I had this in my theme.json settings. What does it mean? Is this a valid combination? At best it's ambiguous; at worst, invalid.
If it's invalid, we can use the set of rules above makes it impossible to type in an invalid state. In this case the
.enabled
option doesn't exist—passing an object ortrue
implies that it is enabled.is the same as (
===
)If
.enabled
is actually the initial state the lightbox value for all images, we should just name it.initial
or.default
and pass an attribute-shaped thing as the value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't enable the lightbox because I hadn't added support for that syntax yet. I wasn't sure how to do it last week and I determined with some other folks to add it later, but just took a look again and believe I got it working in this commit, with a follow up here.
This is indeed a valid combination. The intent here is for
allowEditing
to define whether the UI controls show up, and forenabled
to determine whether the lightbox behavior should work or not.This sounds good to me, but I'll tag in @youknowriad @ramonjd @andrewserong, as they were part of a discussion to define the current structure.
The subtlety here, which I'm not sure is clear, is that we want the lightbox to be disabled by default, but we still want the UI to appear, and also be toggleable. I could see the following structure maybe working:
We'll also likely eventually (probably sooner rather than later) need to add support for an
animation
property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm shooting a follow up on this. @ajlende, would you be alright with the proceeding with the structure as is with
enabled
andallowEditing
? (Summarized below)Note: Using
lightbox: true
will both show the lightbox in the UI and enable the lightbox functionality on the frontend by default. Settinglightbox: false
will remove the lightbox from the UI and disable the lightbox functionality.Block Level Settings Syntax
Top Level Settings Syntax