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

Fix: Button: Replace remaining 40px default size violations [Block library 1] #65033

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

hbhalodia
Copy link
Contributor

Part of - #65018

What?

Why?

  • To make the consistent button across Gutenberg, and we would have a lint rule added once fixed, all the button usage.

How?

  • Change from __next40pxDefaultSize={ false } to __next40pxDefaultSize on component.

Testing Instructions

  • Add the blocks on the page/post and check for the buttons defined.
  • Screenshot is added for individual changed files.

Copy link

github-actions bot commented Sep 4, 2024

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: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

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

@@ -29,8 +29,7 @@ export default function CommentsLegacy( {

const actions = [
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen in comments block, when used in legacy mode.

Before After
Comments-Before Comments-After

Comment on lines +69 to +76
__next40pxDefaultSize
variant="secondary"
onClick={ tryAgain }
>
{ _x( 'Try again', 'button label' ) }
</Button>{ ' ' }
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen on embed block, when you insert a URL that is non-embedded.

Before After
embed-before embed-after

@@ -25,8 +25,7 @@ function ModalAuxiliaryActions( { onClick, isModalFullScreen } ) {

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen while you insert the core/freeform block.

Before After
Classic-before classic-after

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that the Modal close button hasn't had its size updated yet, so we'll need to resolve that first with the design team (#65102).

Could we perhaps revert just this instance (the fullscreen button) to the TODO state, so we can resolve those all together? The other instances in this file are ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the Modal close button hasn't had its size updated yet, so we'll need to resolve that first with the design team (#65102).

Could we perhaps revert just this instance (the fullscreen button) to the TODO state, so we can resolve those all together? The other instances in this file are ok.

Sure @mirka, I would revert this change.

Copy link
Member

Choose a reason for hiding this comment

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

Followed up in #65131

@@ -123,8 +122,7 @@ export default function ModalEdit( props ) {
>
<FlexItem>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen while you insert the core/freeform block.

Before After
Classic-before classic-after

@@ -133,8 +131,7 @@ export default function ModalEdit( props ) {
</FlexItem>
<FlexItem>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen while you insert the core/freeform block.

Before After
Classic-before classic-after

@@ -50,8 +50,7 @@ export default function MissingEdit( { attributes, clientId } ) {

const convertToHtmlButton = (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

@hbhalodia hbhalodia Sep 4, 2024

Choose a reason for hiding this comment

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

This can be seen, when you have a block inserted and that block is no longer registered or available.

Before After
missing-before missing-after

@tyxla tyxla requested a review from a team September 4, 2024 11:59
@tyxla tyxla added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 5, 2024
@mirka mirka changed the title Fix: Button: Replace remaining 40px default size violations Fix: Button: Replace remaining 40px default size violations [Block library 1] Sep 5, 2024
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thank you for helping out with this clean up! I just noticed one issue with the Modal fullscreen button, but everything else looks good 👍

@@ -25,8 +25,7 @@ function ModalAuxiliaryActions( { onClick, isModalFullScreen } ) {

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that the Modal close button hasn't had its size updated yet, so we'll need to resolve that first with the design team (#65102).

Could we perhaps revert just this instance (the fullscreen button) to the TODO state, so we can resolve those all together? The other instances in this file are ok.

This is being reverted because we need to resolve this first - WordPress#65033 (comment)
@hbhalodia
Copy link
Contributor Author

Hi @mirka, I have updated PR the with the requested change. Requesting the re-review for the same.

@hbhalodia hbhalodia requested a review from mirka September 6, 2024 05:32
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Great, thanks 🚀

@mirka mirka merged commit c90f1c5 into WordPress:trunk Sep 6, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 6, 2024
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 11, 2024
…brary 1] (#65033)

* Fix legacy comments to use 40px default button size

* Fix the embed placeholder button to use 40px default button size

* Fix freeform block to use 40px default button size

* Fix missing block to use 40px default button size

* Revert the instance for modal fullscreen button size

This is being reverted because we need to resolve this first - WordPress/gutenberg#65033 (comment)

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

Source: WordPress/gutenberg@c90f1c5
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 12, 2024
…brary 1] (#65033)

* Fix legacy comments to use 40px default button size

* Fix the embed placeholder button to use 40px default button size

* Fix freeform block to use 40px default button size

* Fix missing block to use 40px default button size

* Revert the instance for modal fullscreen button size

This is being reverted because we need to resolve this first - WordPress/gutenberg#65033 (comment)

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

Source: WordPress/gutenberg@c90f1c5
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 13, 2024
…brary 1] (#65033)

* Fix legacy comments to use 40px default button size

* Fix the embed placeholder button to use 40px default button size

* Fix freeform block to use 40px default button size

* Fix missing block to use 40px default button size

* Revert the instance for modal fullscreen button size

This is being reverted because we need to resolve this first - WordPress/gutenberg#65033 (comment)

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

Source: WordPress/gutenberg@c90f1c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants