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.

Sign up for GitHub

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

Jump to bottom

Fix: Button Replace remaining 40px default size violations [Block Directory] #65467

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Fix: Button Replace remaining 40px default size violations [Block Directory] #65467

merged 5 commits into from
Sep 25, 2024

Conversation

vipul0425
Copy link
Contributor

@vipul0425 vipul0425 commented Sep 18, 2024 •
edited by ciampo
Loading

What?

Part of #65018

This would fix in that in subtask block-directory.

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

Testing steps and screenshots are added below.

@github-actions GitHub Actions
Copy link

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

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: vipul0425 <vipulgupta003@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

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

vipul0425
vipul0425 commented Sep 18, 2024
View reviewed changes
packages/block-directory/src/plugins/get-install-missing/index.js
@@ -101,8 +101,7 @@ const ModifiedWarning = ( { originalBlock, ...props } ) => {
);
actions.push(
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before After
keep-as-html-before html-after

vipul0425
vipul0425 commented Sep 18, 2024
View reviewed changes
packages/block-directory/src/plugins/get-install-missing/install-button.js
@@ -22,8 +22,7 @@ export default function InstallButton( { attributes, block, clientId } ) {

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

Choose a reason for hiding this comment

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

Before After
install-block-before install-after

@vipul0425
Copy link
Contributor Author

vipul0425 commented Sep 18, 2024 •
edited
Loading

I have not changed the downloadable-block-list-item button size, as it was causing design issues. If we enable the default 40px size, we will need to override it with custom styles using height: auto. Let me know if it needs to be updated.

Here is the screenshot of design break after adding the __next40pxDefaultSize

downloadable-button-after

@vipul0425 vipul0425 mentioned this pull request Sep 18, 2024
Closed
21 tasks
@mirka mirka added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 19, 2024
@mirka mirka requested a review from a team September 19, 2024 07:35
ciampo
ciampo reviewed Sep 25, 2024
View reviewed changes
packages/block-directory/src/components/downloadable-block-list-item/index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest reviewing this file while hiding whitespace changes, since it will simplify the diff significantly

@ciampo
Copy link
Contributor

ciampo commented Sep 25, 2024

I pushed a commit where I refactored the downloadable block list item component away from Button, since we were overriding completely its styles without using any of its functionality

5bd414a

I tested the changes on my machine and everything seems to be working fine:

Kapture.2024-09-25.at.11.59.32.mp4

ciampo
ciampo reviewed Sep 25, 2024
View reviewed changes
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.

Probably better to get another look from someone else in @WordPress/gutenberg-components given that I authored code in this PR.

@ciampo ciampo requested a review from a team September 25, 2024 10:01
tyxla
tyxla approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good and tests well 👍 Thanks @vipul0425 and @ciampo 🙌

Let's just run npm install to get the package-lock changes committed.

Found a pre-existing glitch - seems like while installing, the "Installing" label is shifted a bit more to the left:

Screen.Recording.2024-09-25.at.18.26.36.mov

Exists in trunk, so likely should be fixed separately.

@ciampo ciampo enabled auto-merge (squash) September 25, 2024 16:12
@ciampo ciampo merged commit 79a8904 into WordPress:trunk Sep 25, 2024
58 of 59 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 25, 2024
@tyxla tyxla mentioned this pull request Sep 26, 2024
Merged
@tyxla
Copy link
Member

tyxla commented Sep 26, 2024

Found a pre-existing glitch - seems like while installing, the "Installing" label is shifted a bit more to the left:

Screen.Recording.2024-09-25.at.18.26.36.mov
Exists in trunk, so likely should be fixed separately.

Fixing this up in #65677.

@mirka mirka added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 1, 2024
@noisysocks noisysocks added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 6, 2024
noisysocks pushed a commit that referenced this pull request Oct 6, 2024
@vipul0425 @ciampo
@tyxla @noisysocks
Fix: Button Replace remaining 40px default size violations [Block Dir…
1ee9fe8 
…ectory] (#65467)

* fix: The button height for install button and keep as HTML.

* Refactor downloadable list item to vanilla button

* Remove unnecessary comment

* Update package-lock.json

---------

Co-authored-by: vipul0425 <vipulgupta003@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
@noisysocks noisysocks mentioned this pull request Oct 6, 2024
Closed
@draganescu draganescu mentioned this pull request Oct 15, 2024
Merged
@sirreal sirreal mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo ciampo left review comments

@tyxla tyxla tyxla approved these changes

Assignees

@vipul0425 vipul0425

Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 19.4
Development

Successfully merging this pull request may close these issues.
5 participants
@vipul0425 @ciampo @tyxla @mirka @noisysocks