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

Add props for buttons in editor 1 #65068

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

AKSHAT2802
Copy link
Contributor

Part of - #65018

What?

Issue - #65018, To use default to 40px for the button.
This would fix blocks like Block Manager, Undo, Redo, error boundary, Post excerpt, Featured Image

Why?

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

How?

Add __next40pxDefaultSize in button on the component.

Testing Instructions

Screenshots added as comment in file changes

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @AKSHAT2802! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@@ -67,8 +67,7 @@ function BlockManager( {
numberOfHiddenBlocks
) }
<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

Style didn't change because there are explicit styles for Reset in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

Copy link
Member

Choose a reason for hiding this comment

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

Note for posterity: There are no explicit height styles for this, just that "link" variant Buttons have height: auto applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

😅

@@ -25,8 +25,7 @@ function EditorHistoryRedo( props, ref ) {
const { redo } = useDispatch( editorStore );
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
undo-redo before undo-redo after

Style didn't change because there are explicit styles for Undo and Redo buttons in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

Copy link
Member

Choose a reason for hiding this comment

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

Note for posterity: In our app code, sizes are added by the consumers.

<ToolbarItem
as={ EditorHistoryUndo }
showTooltip={ ! showIconLabels }
variant={ showIconLabels ? 'tertiary' : undefined }
size="compact"
/>

@@ -30,8 +30,7 @@ function CopyButton( { text, children } ) {
const ref = useCopyToClipboard( text );
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
Error boundary before Error Boundary After

@@ -198,8 +198,7 @@ function PrivateExcerpt() {
ref={ setPopoverAnchor }
renderToggle={ ( { onToggle } ) => (
<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
post-excerpt before Post excerpt after

The style didn't change because there are explicit styles for Add and excerpt in code. But adding __next40pxDefaultSize will ensure that it will not be listed in lint next time

@@ -160,8 +160,7 @@ function PostFeaturedImage( {
render={ ( { open } ) => (
<div className="editor-post-featured-image__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.

Before After
Featured Image before Featured image after

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we can remove most of these style overrides?

.editor-post-featured-image__toggle {
height: 100%;
line-height: 20px;
padding: $grid-unit-10 0;
text-align: center;
box-shadow: inset 0 0 0 $border-width $gray-400;
}

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
Screenshot 2024-09-12 at 1 10 28 AM Screenshot 2024-09-12 at 1 11 21 AM

Hii @mirka removed extra styles
PFA Before and After screenshots

@@ -202,17 +201,15 @@ function PostFeaturedImage( {
{ !! featuredImageId && (
<HStack className="editor-post-featured-image__actions">
<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
replace-remove before replace remove after

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the before/after images are reversed in this one.

We probably need an !important here:

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
Screenshot 2024-09-12 at 12 57 25 AM Screenshot 2024-09-12 at 12 55 49 AM

Hii @mirka , height: auto !important; added for featured image.
PFA new screenshots.

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 a weird flash on focus, but this happens on trunk too so I'll post a separate issue.

CleanShot.2024-09-13.at.05.11.47.mp4

Copy link
Member

Choose a reason for hiding this comment

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

Issue submitted at #65299

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

Looks good, thank you for the follow-ups!

@@ -202,17 +201,15 @@ function PostFeaturedImage( {
{ !! featuredImageId && (
<HStack className="editor-post-featured-image__actions">
<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 a weird flash on focus, but this happens on trunk too so I'll post a separate issue.

CleanShot.2024-09-13.at.05.11.47.mp4

@mirka mirka merged commit a895274 into WordPress:trunk Sep 12, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants