-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 2 #65083
Add props for buttons in editor 2 #65083
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 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. |
@@ -100,8 +100,7 @@ export default function PostFormat() { | |||
{ suggestion && suggestion.id !== postFormat && ( | |||
<p className="editor-post-format__suggestion"> | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @mirka mentioned in another PR, variant="link"
Button
components have their height set to auto
, and therefore the __next40pxDefaultSize
doesn't make any difference
variant="tertiary" | ||
href={ unlockUrl } | ||
> | ||
{ __( 'Take over' ) } | ||
</Button> | ||
) } | ||
<Button | ||
// TODO: Switch to `true` (40px size) if possible | ||
__next40pxDefaultSize={ false } | ||
__next40pxDefaultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -25,8 +25,7 @@ const PostFormatSuggestion = ( { | |||
onUpdatePostFormat, | |||
} ) => ( | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -155,8 +155,7 @@ export default function PostFormatPanel() { | |||
<Spinner /> | |||
) : ( | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -36,8 +36,7 @@ function PostLastRevision() { | |||
return ( | |||
<PostLastRevisionCheck> | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
__next40pxDefaultSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to find how to test this instance in the editor — it looks like the PostLastRevision
component is not used anymore since #62323 (cc @ntsekouras ) but we're still exporting it.
In any case, I can see that there are some style overrides in place that tweak the height
, and so passing the __next40pxDefaultSize
prop shouldn't affect the look of this button.
For the scope of this PR, I think that we can leave things as they are.
As a future improvement, we should avoid overriding button (and panel) styles. If we feel like we're tweaking Button
beyond its purpose, we should consider not using Button
at all (cc @mirka )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this component is not used any more in favor of PrivatePostLastRevision
component. There was some discussions whether to remove it here and we ended up keeping it for existing consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it still makes sense to remove the overrides if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed — it would be good to do that as a follow up to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A follow-up PR sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
Apart from #65083 (comment), we will also need to update test snapshots
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
561c6ff
to
3187318
Compare
Code changes are looking good, but we'll need to update snapshots in order to have passing unit tests |
Hii @ciampo, I have updated test snapshots for the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 ✅
Thanks for working on it @AKSHAT2802 🙌
Part of - #65018
What?
Issue - #65018, To use default to 40px for the button.
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
Screenshot is added for individual changed files.