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 violation [Block library 4] #65143

Merged
merged 6 commits into from
Sep 10, 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.

@@ -177,8 +175,7 @@ function SingleTrackEditor( { track, onChange, onClose, onRemove } ) {
{ __( 'Close' ) }
</Button>
<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.

This can be seen while you add the core/video and add some track in track list.

Note: This would have no impact, as it have link style for the button. Still as a part of to make button consistent, we have added this change.

Before After
Remove-track-before remove-track-after

@@ -147,8 +146,7 @@ function SingleTrackEditor( { track, onChange, onClose, onRemove } ) {
/>
<HStack className="block-library-video-tracks-editor__single-track-editor-buttons-container">
<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.

This can be seen, when you add core/video and add some track list and open in edit mode.

Before After
Close-track-before close-track-after

@@ -58,8 +58,7 @@ function TrackList( { tracks, onEditPress } ) {
>
<span>{ track.label } </span>
<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.

This can be seen when you add core/video block and add a track list and check for the edit button.

Before After
Edit-track-before Edit-track-after

@@ -262,8 +261,7 @@ function VideoEdit( {
</p>
{ !! poster && (
<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.

This can be seen while you add core/video, and check on inspector control while you add video poster image and after adding check for the remove button.

Before After
video-poster-before-remove video-poster-after-remove

@@ -232,8 +232,7 @@ function VideoEdit( {
}
render={ ( { open } ) => (
<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.

This can be seen while you add core/video, and check on inspector control while you need to add a poster image.

Before After
Video-edit-before video-edit-after

@@ -86,8 +85,7 @@ export default function TemplatePartPlaceholder( {

{ ! isResolving && isBlockBasedTheme && canCreateTemplatePart && (
<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.

This can be seen, while you add core/template-part and check for the placeholder button.

Before After
template-part-before template-part-after

@@ -75,8 +75,7 @@ export default function TemplatePartPlaceholder( {
{ ! isResolving &&
!! ( templateParts.length || blockPatterns.length ) && (
<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.

This can be seen, while you add core/template-part and check for the placeholder button.

Before After
template-part-before template-part-after

@@ -89,8 +88,7 @@ export default function QueryPlaceholder( {
) }

<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.

This can be seen while you add the core/query block and check for the button Start blank.

Before After
placeholder-before placeholder-after

@@ -79,8 +79,7 @@ export default function QueryPlaceholder( {
>
{ !! hasPatterns && (
<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.

This can be seen while you add the core/query block and check for the button Choose.

Before After
placeholder-before placeholder-after

@@ -76,8 +76,7 @@ export default function EnhancedPaginationModal( {
<VStack alignment="right" spacing={ 5 }>
<span id={ modalDescriptionId }>{ notice }</span>
<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.

Hi @mirka @ciampo, I was not able to get this visually on the editor, tried some hacks as well, like to directly call this when adding the block, but did not get any success.

Can you help with this? All others screenshot has been attached.

Thank You,

Copy link
Member

Choose a reason for hiding this comment

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

I was able to force it to show by temporarily changing this isOpen condition to true:

Looks ok 👍

Enhanced pagination modal

@hbhalodia hbhalodia marked this pull request as ready for review September 10, 2024 05:19
Copy link

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>

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

@mirka mirka added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 10, 2024
@mirka mirka requested a review from a team September 10, 2024 10:33
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.

Everything looks good, thanks!

@@ -76,8 +76,7 @@ export default function EnhancedPaginationModal( {
<VStack alignment="right" spacing={ 5 }>
<span id={ modalDescriptionId }>{ notice }</span>
<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 was able to force it to show by temporarily changing this isOpen condition to true:

Looks ok 👍

Enhanced pagination modal

@mirka mirka merged commit ff0c475 into WordPress:trunk Sep 10, 2024
79 of 83 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 10, 2024
@hbhalodia
Copy link
Contributor Author

Ah! great, thanks. Not sure why that does not work for me. I tried same by forcing it to true, nvm thanks for review 🚀.

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.

2 participants