Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
e613a5b
Add initial implementation of image settings panel
artemiomorales Aug 30, 2023
b0fb3b5
Remove unnecessary code and fix reset functionality
artemiomorales Aug 30, 2023
b76d56c
Add UI to image block inspector
artemiomorales Aug 30, 2023
1872f77
Add `lightbox` to the valid `theme.json` settings
michalczaplinski Aug 31, 2023
3cfcb15
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Aug 31, 2023
d2818af
Fix a bug with image selector and integrate with global styles
michalczaplinski Aug 31, 2023
a32d0f6
Update `theme.json` schema
michalczaplinski Sep 2, 2023
e67634e
Added the `@since`annotation
michalczaplinski Sep 2, 2023
3ddb1b1
Refactor image settings panel and screen block
michalczaplinski Sep 4, 2023
fae6f99
Add showUI option to lightbox settings
michalczaplinski Sep 4, 2023
df9f857
Add defaults for the `lightbox` to the GB `theme.json`
michalczaplinski Sep 4, 2023
df3f1c6
Change the falsy checks in image.js
michalczaplinski Sep 4, 2023
075535b
Add filters; add legacy support for behaviors syntax
artemiomorales Sep 5, 2023
c2a0b3f
Fix linter errors & add more expansive comments.
michalczaplinski Sep 5, 2023
91d130c
If no value is set for the lightbox in the Global Styles, then the bl…
michalczaplinski Sep 5, 2023
c58612c
Rename `showUI` to `allowEditing`
michalczaplinski Sep 5, 2023
c602139
Fix the `theme.json` schema
michalczaplinski Sep 5, 2023
339a715
Use the globalPath for the settings on the PHP side
michalczaplinski Sep 5, 2023
4b43b30
Add backwards support for enabling fade animation via the legacy syntax
artemiomorales Sep 6, 2023
ce3c338
Fix PHP linter errors
artemiomorales Sep 6, 2023
7fefb83
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Sep 6, 2023
03e60d7
Fix error when checking lightbox['enabled'] value in global settings
artemiomorales Sep 6, 2023
050c636
Empty commit
michalczaplinski Sep 6, 2023
521d950
Remove the default `false` value for `lightbox.enabled`attribute.
michalczaplinski Sep 7, 2023
1db73c5
Add deprecation notice for 'behaviors' in image block
artemiomorales Sep 7, 2023
715e32c
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Sep 7, 2023
41f251d
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Sep 7, 2023
79dad34
Add deprecation for attribute in image block
artemiomorales Sep 9, 2023
871ba45
Remove obsolete code now that block deprecation is in place
artemiomorales Sep 10, 2023
7f2f70f
Add support for 'lightbox: true' syntax
artemiomorales Sep 11, 2023
6da3545
Fix lightbox 'checked' attribute being read improperly
artemiomorales Sep 11, 2023
83bef30
Add conditional display for settings panel at image block level
artemiomorales Sep 11, 2023
52e358d
Fix an error with the theme.json schema.
michalczaplinski Sep 11, 2023
11e96e2
Update docs with `npm run build:docs`
michalczaplinski Sep 11, 2023
8809612
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Sep 11, 2023
59e0dbe
Copy `class-wp-theme-json-schema.php` from core
michalczaplinski Sep 11, 2023
3eeb1a7
Merge branch 'update/lightbox-settings-panels' of github.com:WordPres…
michalczaplinski Sep 11, 2023
443aa5b
Add a theme.json migration to v3 away from behaviors and to a new, si…
michalczaplinski Sep 11, 2023
6459d56
Remove the `null` value from lightbox.enabled in `lib/theme.json`
michalczaplinski Sep 11, 2023
203649d
Remove `behaviors` from VALID_TOP_LEVEL_KEYS & VALID_SETTINGS
michalczaplinski Sep 11, 2023
5a37499
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
artemiomorales Sep 11, 2023
0e63890
Revise backwards compatibility for behaviors; add deprecation to bloc…
artemiomorales Sep 11, 2023
8b721a7
Remove outdated comment
michalczaplinski Sep 12, 2023
8287c46
Update outdated comment
michalczaplinski Sep 12, 2023
a2c2b94
Update comment and shuffle the lines so the diff is easier on the eyes.
michalczaplinski Sep 12, 2023
4f0709b
Update comment to explain why we use userSettings in image-settings-p…
michalczaplinski Sep 12, 2023
7018ee7
Remove support for legacy fade configuration
artemiomorales Sep 12, 2023
7fd0801
Resolve lint error
artemiomorales Sep 12, 2023
5bb05bc
Add clarifying comment regarding lightbox markup
artemiomorales Sep 12, 2023
a02910c
Rename the migrate function to reflect that it's not a v3 migration
michalczaplinski Sep 12, 2023
b12d386
Add a `@since` in `gutenberg_should_render_lightbox` docblock
michalczaplinski Sep 12, 2023
2f5f122
Add support for reading top-level 'lightbox' setting in editor
artemiomorales Sep 12, 2023
a238d94
Revert "Add support for reading top-level 'lightbox' setting in editor"
artemiomorales Sep 13, 2023
0aba3c4
Add correct deprecation mentioning the Gutenberg version
michalczaplinski Sep 13, 2023
8fa991e
Move 'allowEditing' to top-level settings
artemiomorales Sep 13, 2023
65b173c
Fix top-level lightbox setting not being read properly
artemiomorales Sep 13, 2023
b5f8186
Fix 'false' values in theme.json not being stored in settings object
artemiomorales Sep 13, 2023
7e121da
Fix error wherein 'undefined' was being passed to input component
artemiomorales Sep 13, 2023
fb10384
Fix bug wherein lightbox UI would disappear if user value was set
artemiomorales Sep 13, 2023
ef8f43e
Remove inheritance when determining whether to enable lightbox
artemiomorales Sep 13, 2023
fc8fc47
Update comment
artemiomorales Sep 13, 2023
6ea39f8
Update whitespace in theme.json
michalczaplinski Sep 14, 2023
aaab1ef
Add docblocks to clarify that `class-wp-theme-json-schema-gutenberg.p…
michalczaplinski Sep 14, 2023
61bfb3d
Add comments to clarify that behaviors.php is temporarily added to GB…
michalczaplinski Sep 14, 2023
c18e283
Added integration fixtures for the lightbox
michalczaplinski Sep 14, 2023
7d30aa6
Merge branch 'update/revise-lightbox-ui' into update/lightbox-setting…
michalczaplinski Sep 14, 2023
dab7f5a
Fix incorrect reading of global lightbox settings
artemiomorales Sep 14, 2023
a5c2f0c
Clarify the comment getting the global lightbox settings
michalczaplinski Sep 14, 2023
485c197
Fix PHPCS parenthesis error 🤦‍♂️
michalczaplinski Sep 15, 2023
d1432bc
Move lightbox settings to the block level
artemiomorales Sep 15, 2023
ff1d2b5
Remove support for shorthand
artemiomorales Sep 15, 2023
ee1dd8c
Remove 'lightbox' from hooks.js and add `.enabled` & `.allowEditing` …
michalczaplinski Sep 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
If no value is set for the lightbox in the Global Styles, then the bl…
…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.
  • Loading branch information
michalczaplinski committed Sep 5, 2023
commit 91d130c12fa99fab9533baf69fa3249ce1995b2e
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export function useHasImageSettingsPanel( name, settings ) {
export default function ImageSettingsPanel( {
onChange,
userSettings,
settings,
panelId,
} ) {
const resetLightbox = () => {
Expand Down Expand Up @@ -43,10 +44,7 @@ export default function ImageSettingsPanel( {
>
<ToggleControl
label={ __( 'Expand on Click' ) }
// TODO: Figure out if we want to use the `userSettings` or the global
// settings. If we use the global settings, this means that the
// checkbox will be checked following the `theme.json` value.
checked={ !! userSettings?.lightbox?.enabled }
checked={ !! settings?.lightbox?.enabled }
onChange={ onChangeLightbox }
/>
</ToolsPanelItem>
Expand Down
16 changes: 15 additions & 1 deletion packages/block-library/src/image/image.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
__experimentalImageURLInputUI as ImageURLInputUI,
MediaReplaceFlow,
store as blockEditorStore,
useSetting,
BlockAlignmentControl,
__experimentalImageEditor as ImageEditor,
__experimentalGetElementClassName,
Expand Down Expand Up @@ -173,6 +174,7 @@ export default function Image( {
},
[ clientId ]
);

const { replaceBlocks, toggleSelection } = useDispatch( blockEditorStore );
const { createErrorNotice, createSuccessNotice } =
useDispatch( noticesStore );
Expand Down Expand Up @@ -367,6 +369,18 @@ export default function Image( {
availableUnits: [ 'px' ],
} );

const lightboxSetting = useSetting( 'lightbox' );

const lightboxChecked = useMemo( () => {
// If the lightbox is set in the block attributes, use that.
if ( lightbox?.enabled ) return true;

// If the lightbox is NOT set in the block attributes AND it IS enabled in
// the settings, use that.
if ( ! lightbox && lightboxSetting?.enabled ) return true;
return false;
}, [ lightbox, lightboxSetting ] );
Copy link
Contributor

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.

Suggested change
const lightboxChecked = useMemo( () => {
// If the lightbox is set in the block attributes, use that.
if ( lightbox?.enabled ) return true;
// If the lightbox is NOT set in the block attributes AND it IS enabled in
// the settings, use that.
if ( ! lightbox && lightboxSetting?.enabled ) return true;
return false;
}, [ lightbox, lightboxSetting ] );
const lightboxChecked =
lightbox?.enabled || ( ! lightbox && lightboxSetting?.enabled );

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:

  • A feature with options is fully disabled by default.
  • It has a set of default options that get set with { "appearanceTools": true }.
  • The feature can be enabled with default options by setting the feature as true (as above).
  • Or the options can be configured individually by passing the object.
  • Any missing options use the defaults.

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:

<!-- {} -->
<!-- `undefined` values are stripped in block attributes, so this would be equivalent to -->
<!-- { "lightbox": undefined } -->

Moving on to theme.json settings. This example confuses me:

// theme.json
{
	"lightbox": {
		"enabled": false,
		"allowEditing": true
	}
}

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 or true implies that it is enabled.

// theme.json
{
	"lightbox": {
		"allowEditing": true
	}
}

is the same as (===)

// theme.json
{
	"lightbox": true
}

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<!-- { "lightbox": true } -->
Unexpectedly, this does not enable the lightbox.

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.

Moving on to theme.json settings. This example confuses me:

// theme.json

{
  "lightbox": {
  	"enabled": false,
  	"allowEditing": true
  }
}

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.

This is indeed a valid combination. The intent here is for allowEditing to define whether the UI controls show up, and for enabled to determine whether the lightbox behavior should work or not.

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.

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:

{
	"lightbox": {
		"allowEditing": true
		"default": {
			"enabled": false
		}
	}
}

We'll also likely eventually (probably sooner rather than later) need to add support for an animation property.

Copy link
Contributor Author

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 and allowEditing? (Summarized below)

Note: Using lightbox: true will both show the lightbox in the UI and enable the lightbox functionality on the frontend by default. Setting lightbox: false will remove the lightbox from the UI and disable the lightbox functionality.

Block Level Settings Syntax

"settings": {
	"blocks": {
		"core/image": {
			"lightbox": {
				"enabled": boolean
				"allowEditing": boolean
			}
		}
	}
}
"settings": {
	"blocks": {
		"core/image": {
			"lightbox": boolean
		}
	}
}

Top Level Settings Syntax

"settings": {
	"lightbox": {
		"enabled": boolean
		"allowEditing": boolean
	}
}
"settings": {
	"lightbox": boolean
}


const controls = (
<>
<BlockControls group="block">
Expand Down Expand Up @@ -532,7 +546,7 @@ export default function Image( {
>
<ToggleControl
label={ __( 'Expand on Click' ) }
checked={ !! lightbox?.enabled }
checked={ lightboxChecked }
onChange={ ( newValue ) => {
setAttributes( {
lightbox: { enabled: newValue },
Expand Down
24 changes: 20 additions & 4 deletions packages/edit-site/src/components/global-styles/screen-block.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,25 @@ function ScreenBlock( { name, variation } ) {
}
};
const onChangeLightbox = ( newSetting ) => {
setSettings( {
...rawSettings,
lightbox: newSetting,
} );
// If the newSetting is undefined, this means that the user has deselected
// (reset) the lightbox setting.
if ( newSetting === undefined ) {
setSettings( {
...rawSettings,
lightbox: undefined,
} );

// Otherwise, we simply set the lightbox setting to the new value but
// taking care of not overriding the other lightbox settings.
} else {
setSettings( {
...rawSettings,
lightbox: {
...rawSettings.lightbox,
...newSetting,
},
} );
}
};
const onChangeBorders = ( newStyle ) => {
if ( ! newStyle?.border ) {
Expand Down Expand Up @@ -265,6 +280,7 @@ function ScreenBlock( { name, variation } ) {
<ImageSettingsPanel
onChange={ onChangeLightbox }
userSettings={ userSettings }
settings={ settings }
/>
) }

Expand Down