Media Replace Flow: Add custom toggle support and fix button height #68084

yogeshbhutkar · 2024-12-18T09:46:56Z

What?

This PR contains a patch for fixing spacing inconsistency in the background image selector button.

How?

As the issue description mentions, the default size of Toolbar Button was updated to compact, whose styles are applied using the is-compact classname.

The is-compact classname contains a height of 32px as mentioned here:

gutenberg/packages/components/src/button/style.scss

Line 298 in 581de9f

height: $button-size-compact;

gutenberg/packages/base-styles/_variables.scss

Line 99 in 581de9f

$button-size-compact: 32px;

Whereas, the heights described in .components-dropdown applied to its parent, is actually 36px leading to a mismatch in the parent container and child's heights.

gutenberg/packages/block-editor/src/components/background-image-control/style.scss

Line 26 in 581de9f

height: 36px;

gutenberg/packages/block-editor/src/components/background-image-control/style.scss

Line 47 in 581de9f

height: 36px;

To fix the issue, the heights are updated inside .components-dropdown to now use fit-content instead of using the previously hardcoded value 36px.

As an alternative, we can also hardcode the height to 32px.

File Changed:

> packages/block-editor/src/components/background-image-control/style.scss

Testing Instructions

Go to the site editor.
Click on "Styles"
Click on "Background"
Confirm that the spacing for the button is now correct.

Screenshots

Before	After

…te BackgroundImageControls to specify button variant

…and remove unused default variant

github-actions · 2024-12-18T09:47:38Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

yogeshbhutkar · 2024-12-18T10:06:35Z

Requesting a review from @mirka, as this is being carried forward from the previous PR: #68038

Apologies for any inconvenience this may have caused.

tyxla

My feedback from #68038 has been addressed, so I'll let @mirka review individually when she gets a chance.

mirka

This is good, and I think we can future-proof it even further by exposing the entire renderToggle as an optional prop, rather than try to abstract it with buttonVariant. My concern with an abstract approach like buttonVariant is that we'll need to alter the API every time we need something slightly different, like an icon button or another Button variant.

So for example in BackgroundImageControls I want to be able to do something like:

<MediaReplaceFlow
  renderToggle={ ( props ) => (
    <Button
      { ...props }
      __next40pxDefaultSize
    />
  ) }

What do you think? (cc @WordPress/gutenberg-components)

Also as a separate issue, I noticed that the focus manipulation that editMediaButtonRef is being used to do is not relevant/appropriate anymore in these recent implementations. I'm looking at the original commit where that focus manipulation was added, and it seems like it was for focus restoration, which doesn't apply anymore (cc @draganescu in case I'm wrong). We could maybe clean that up as well.

mirka · 2024-12-19T16:47:35Z

packages/block-editor/src/components/background-image-control/style.scss

@@ -23,7 +23,7 @@

 	.components-dropdown {
 		display: block;
-		height: 36px;
+		height: fit-content;


Can these height rules be removed entirely?

Suggested change

height: fit-content;

Thanks for the review @mirka.

Yes, we can indeed remove the height rules. I've removed them in the latest commit. Here's a screenshot of the component after recent changes:

It's working as expected on my end.

This is good, and I think we can future-proof it even further by exposing the entire renderToggle as an optional prop, rather than try to abstract it with buttonVariant.

Great Point! I'll work on incorporating this, will update here once done.

…wn styles

…on rendering and support custom toggle rendering

yogeshbhutkar · 2024-12-20T05:37:09Z

As per the suggestions provided by @mirka, the following are the changes made in the recent commit:

Removed the code for buttonVariant abstract approach in favor of introducing the exposure of renderToggle as an optional prop.
Cleaned up editMediaButtonRef as it was referred not relevant in the recent implementations here.
Removed name prop from being passed inside BackgroundImageControls component when using the MediaReplaceFlow as when renderToggle is provided, the name is also provided within the Button. This name is anyways supposed to be used by the Button itself, so we can save up some redundancy by passing it within renderToggle itself.

Here's a screencast post recent changes:

The changes seem to work as expected on my end.

tyxla · 2024-12-20T15:03:42Z

packages/block-editor/src/components/background-image-control/index.js

+						aria-haspopup="true"
+						onClick={ onToggle }
+						onKeyDown={ ( event ) => {
+							if ( event.keyCode === DOWN ) {


I know we've been doing it like that in other places, but event.keyCode is deprecated. You might want to use:

Suggested change

if ( event.keyCode === DOWN ) {

if ( event.key === 'ArrowDown' ) {

Hi @tyxla, thanks for the review. I've incorporated the suggested changes in the latest commit.

…vigation

packages/block-editor/src/components/media-replace-flow/index.js

Mamaduka · 2024-12-24T16:01:58Z

packages/block-editor/src/components/media-replace-flow/index.js

+			aria-expanded={ isOpen }
+			aria-haspopup="true"
+			onClick={ onToggle }
+			onKeyDown={ openOnArrowDown }


I wonder if we should pass down these as default props for the media flow render toggle. This way, consumers don't have to re-implement a11y best practices.

Exactly, that is what I had in mind with this usage example. We can assume (and document) that the renderToggle function should accept and pass through button props to an eventual button element.

@yogeshbhutkar Let me know if you're unsure what to do here, I'd be happy to push up some proposed code.

@mirka, yeah, reread your example after I commented and noticed it 😅

mirka · 2024-12-26T12:33:08Z

packages/block-editor/src/components/media-replace-flow/index.js

+			aria-expanded={ isOpen }
+			aria-haspopup="true"
+			onClick={ onToggle }
+			onKeyDown={ openOnArrowDown }


Exactly, that is what I had in mind with this usage example. We can assume (and document) that the renderToggle function should accept and pass through button props to an eventual button element.

@yogeshbhutkar Let me know if you're unsure what to do here, I'd be happy to push up some proposed code.

mirka · 2024-12-26T12:40:03Z

packages/block-editor/src/components/background-image-control/index.js

-				name={
-					<InspectorImagePreviewItem
-						className="block-editor-global-styles-background-panel__image-preview"
-						imgUrl={ url }
-						filename={ title }
-						label={ imgLabel }
-					/>


Because name is a standard prop of MediaReplaceFlow, I think it would be safer to continue using this prop rather than rendering it directly into our renderToggle.

One risk I'm thinking about is that the MediaReplaceFlow implementation could change some day to use the name prop in another place as well.

yogeshbhutkar · 2024-12-27T05:35:49Z

While implementing the suggested changes, I found that entirely removing this CSS rule changes the height of the Image Preview component. Instead of completely removing it, the following rule will fix the issue:

gutenberg/packages/block-editor/src/components/background-image-control/style.scss

Line 27 in e3df6a0

.block-editor-global-styles-background-panel__dropdown-toggle {

Before	After

Also, it looks like there were some missed lints in the JSON file that previously got committed to the latest trunk

gutenberg/packages/block-library/src/post-template/block.json

Line 53 in a4aac83

"padding" : false,

The pre-commit hook seems to have fixed those instances (#64425) in this PR while pushing the merged code to resolve conflicts.

Finally, this comment clarified the suggested approach further. So, I've updated the renderToggle implementation to accept button props, which must be passed to the button component. I've also documented this change.

If you feel this is not the best approach, I can refer to a small code example.

Here's a screencast after the suggested changes:

Thanks a lot to the reviewers for these constant follow-ups ✨.

CC: @Mamaduka @mirka

packages/block-library/src/post-template/block.json

Mamaduka · 2024-12-27T15:44:35Z

packages/block-editor/src/components/background-image-control/style.scss

Are changes in this file still necessary?

Yes, @Mamaduka. I've tried to explain it above in this comment.

Thanks a lot for reviewing the PR 🚀

mirka

API changes look great, thank you! I think we're good to go once the height value is fixed.

mirka · 2024-12-27T17:45:58Z

packages/block-editor/src/components/background-image-control/style.scss

-		height: 36px;
+
+		.block-editor-global-styles-background-panel__dropdown-toggle {
+			height: 36px;


Suggested change

height: 36px;

height: 40px;

Thanks for the review @mirka. I've updated the height in the latest commit.

mirka

Thanks for all the follow-ups! 🚀

…ordPress#68084) * Update background image control dropdown height to fit-content * Replace ToolbarButton with Button in MediaReplaceFlow component * Refactor MediaReplaceFlow to support dynamic button variants and update BackgroundImageControls to specify button variant * Refactor MediaReplaceFlow to set default button variant to 'toolbar' and remove unused default variant * Remove redundant height property from background image control dropdown styles * Refactor BackgroundImageControls and MediaReplaceFlow to improve button rendering and support custom toggle rendering * Remove unnecessary blank line in MediaReplaceFlow component * Update BackgroundImageControls to use 'ArrowDown' key for dropdown navigation * Media Replace Flow: Add custom toggle support and fix button height * added `renderToggle` prop details to readme * refactor: remove unused styles * style: increase dropdown toggle height to 40px Co-authored-by: yogeshbhutkar <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: juanfra <[email protected]>

yogeshbhutkar added 4 commits December 18, 2024 15:12

Update background image control dropdown height to fit-content

c55c3b6

Replace ToolbarButton with Button in MediaReplaceFlow component

9894d75

Refactor MediaReplaceFlow to support dynamic button variants and upda…

ca288fe

…te BackgroundImageControls to specify button variant

Refactor MediaReplaceFlow to set default button variant to 'toolbar' …

f8983b1

…and remove unused default variant

yogeshbhutkar marked this pull request as ready for review December 18, 2024 09:47

yogeshbhutkar requested a review from ellatrix as a code owner December 18, 2024 09:47

yogeshbhutkar mentioned this pull request Dec 18, 2024

Update background image control dropdown height to fit-content #68038

Closed

juanfra added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor labels Dec 18, 2024

juanfra requested a review from mirka December 18, 2024 10:28

tyxla reviewed Dec 18, 2024

View reviewed changes

mirka reviewed Dec 19, 2024

View reviewed changes

yogeshbhutkar added 2 commits December 20, 2024 08:58

Remove redundant height property from background image control dropdo…

e2295f0

…wn styles

Refactor BackgroundImageControls and MediaReplaceFlow to improve butt…

a0b5844

…on rendering and support custom toggle rendering

Remove unnecessary blank line in MediaReplaceFlow component

96a625a

ciampo requested review from mirka and tyxla December 20, 2024 11:19

tyxla reviewed Dec 20, 2024

View reviewed changes

yogeshbhutkar added 2 commits December 23, 2024 14:42

Update BackgroundImageControls to use 'ArrowDown' key for dropdown na…

e1ec285

…vigation

Merge branch 'trunk' into 68028/fix-spacing-issue

f37e830

Mamaduka reviewed Dec 24, 2024

View reviewed changes

packages/block-editor/src/components/media-replace-flow/index.js Show resolved Hide resolved

Mamaduka reviewed Dec 24, 2024

View reviewed changes

mirka reviewed Dec 26, 2024

View reviewed changes

yogeshbhutkar added 3 commits December 27, 2024 10:09

Media Replace Flow: Add custom toggle support and fix button height

92855a8

added renderToggle prop details to readme

dfe66b2

Merge branch 'trunk' into 68028/fix-spacing-issue

eee0103

yogeshbhutkar requested a review from ajitbohra as a code owner December 27, 2024 04:54

yogeshbhutkar requested a review from fabiankaegy as a code owner December 27, 2024 04:54

refactor: remove unused styles

e3df6a0

yogeshbhutkar requested review from mirka and Mamaduka December 27, 2024 06:00

Mamaduka reviewed Dec 27, 2024

View reviewed changes

packages/block-library/src/post-template/block.json Outdated Show resolved Hide resolved

Mamaduka reviewed Dec 27, 2024

View reviewed changes

mirka reviewed Dec 27, 2024

View reviewed changes

style: increase dropdown toggle height to 40px

d2e6e3e

yogeshbhutkar changed the title ~~Update background image control dropdown height to fit-content~~ Media Replace Flow: Add custom toggle support and fix button height Dec 30, 2024

yogeshbhutkar requested a review from mirka December 30, 2024 06:55

mirka approved these changes Jan 2, 2025

View reviewed changes

mirka merged commit 9b35bc6 into WordPress:trunk Jan 2, 2025
64 checks passed

github-actions bot added this to the Gutenberg 20.0 milestone Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media Replace Flow: Add custom toggle support and fix button height #68084

Media Replace Flow: Add custom toggle support and fix button height #68084

yogeshbhutkar commented Dec 18, 2024

github-actions bot commented Dec 18, 2024 •

edited

Loading

yogeshbhutkar commented Dec 18, 2024 •

edited

Loading

tyxla left a comment

mirka left a comment

mirka Dec 19, 2024

yogeshbhutkar Dec 20, 2024 •

edited

Loading

yogeshbhutkar commented Dec 20, 2024

tyxla Dec 20, 2024

yogeshbhutkar Dec 23, 2024

Mamaduka Dec 24, 2024

mirka Dec 26, 2024

Mamaduka Dec 26, 2024

mirka Dec 26, 2024

mirka Dec 26, 2024

yogeshbhutkar commented Dec 27, 2024

Mamaduka Dec 27, 2024

yogeshbhutkar Dec 30, 2024

mirka left a comment

mirka Dec 27, 2024

yogeshbhutkar Dec 30, 2024

mirka left a comment

	if ( event.keyCode === DOWN ) {
	if ( event.key === 'ArrowDown' ) {

Media Replace Flow: Add custom toggle support and fix button height #68084

Media Replace Flow: Add custom toggle support and fix button height #68084

Conversation

yogeshbhutkar commented Dec 18, 2024

What?

How?

File Changed:

Testing Instructions

Screenshots

github-actions bot commented Dec 18, 2024 • edited Loading

yogeshbhutkar commented Dec 18, 2024 • edited Loading

tyxla left a comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogeshbhutkar Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

yogeshbhutkar commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogeshbhutkar commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024 •

edited

Loading

yogeshbhutkar commented Dec 18, 2024 •

edited

Loading

yogeshbhutkar Dec 20, 2024 •

edited

Loading