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

FocalPointPicker with __next40pxDefaultSize #56021

Merged
merged 4 commits into from Dec 7, 2023
Merged

Conversation

richtabor
Copy link
Member

What?

Part of the effort towards #46734.

Testing Instructions

  1. Open a post or page.
  2. Insert a cover block.
  3. Set an image.
  4. Open block inspector.
  5. See FocalPointPicker

Screenshots or screencast

Before After
CleanShot 2023-11-09 at 18 00 34 CleanShot 2023-11-09 at 17 59 36

@richtabor richtabor added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Nov 9, 2023
@richtabor richtabor requested review from ciampo and a team November 9, 2023 23:02
@richtabor richtabor self-assigned this Nov 9, 2023
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Early green check on this one.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Good to be merged after adding a CHANGELOG entry 🚀

@ciampo
Copy link
Contributor

ciampo commented Nov 13, 2023

Actually, @brookewp (who has been recently working on updating some components to the new 40px height) rightfully noted that we should add the __next40pxDefaultSize on FocalPointPicker, so that we continue the same approach used with other components. @richtabor would you be able to do that? Thank you!

@richtabor
Copy link
Member Author

Actually, @brookewp (who has been recently working on updating some components to the new 40px height) rightfully noted that we should add the __next40pxDefaultSize on FocalPointPicker, so that we continue the same approach used with other components.

Ah, so instead of setting __next40pxDefaultSize on the StyledUnitControl, it should be moved to the FocalPointPicker and passed down?

@brookewp
Copy link
Contributor

Ah, so instead of setting __next40pxDefaultSize on the StyledUnitControl, it should be moved to the FocalPointPicker and passed down?

Yes, exactly. Similarly to how __nextHasNoMarginBottom is added and passed down to FocalPointPickerControls.

That way, the size can be updated by opting in to use the prop. Otherwise, it'll stay the same since the default value will be false for now. We can then do a follow-up to update all of the usages of <FocalPointPicker/> in GB.

There are some recent examples of how this was done with other components here: #55471, #55789, and #53819 - I'm also happy to help if needed!

@richtabor
Copy link
Member Author

Thanks @brookewp, feel free to jump in here if you'd like to. I don't mind! :)

@brookewp brookewp requested review from ciampo and a team November 21, 2023 03:40
@ciampo ciampo marked this pull request as ready for review November 22, 2023 15:40
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Thank you all for the collaboration!

@ciampo
Copy link
Contributor

ciampo commented Dec 4, 2023

We're good to merge!

Copy link

github-actions bot commented Dec 7, 2023

Flaky tests detected in 718a033.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7121747313
📝 Reported issues:

@brookewp brookewp merged commit 72280c1 into trunk Dec 7, 2023
52 checks passed
@brookewp brookewp deleted the tweak/focal-point-40 branch December 7, 2023 00:54
@github-actions github-actions bot added this to the Gutenberg 17.3 milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants