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 [Edit Site 2] #65258

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

@@ -28,8 +28,7 @@ function FontCard( { font, onClick, variantsText, navigatorPath } ) {

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.

This can be seen when you visit, site-editor --> styles --> typogrpahy --> open font library modal.

Note: This has changed some styles, but that is not looking good, should we add some higher specificity or !important? as we did previously with block library? @mirka @ciampo

Before After
Font-library-modal-before font-librayr-modal-after

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I guess we can go with a !important on the height: auto?

Copy link
Member

Choose a reason for hiding this comment

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

(FYI, unrelated to this but the tabs styling was broken. Fix proposed in #65330)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess we can go with a !important on the height: auto?

Sure, Would do this.

@@ -472,8 +472,7 @@ function FontCollection( { slug } ) {
className="font-library-modal__footer"
>
<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 visit, site-editor --> styles --> typogrpahy --> open font library modal and install fonts tab.

Before After
install-fonts-before install-fonts-after

@@ -437,8 +437,7 @@ function InstalledFonts() {
{ isInstalling && <ProgressBar /> }
{ shouldDisplayDeleteButton && (
<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 visit, site-editor --> styles --> typogrpahy --> open font library modal and choose font to edit.

Before After
installed-fonts-delete-before Screenshot 2024-09-12 at 11 04 50 AM

@@ -447,8 +446,7 @@ function InstalledFonts() {
</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 when you visit, site-editor --> styles --> typogrpahy --> open font library modal and choose font

Before After
installed-fonts-update-before installed-fonts-update-after

@@ -230,8 +230,7 @@ function UploadFonts() {
onChange={ onFilesUpload }
render={ ( { openFileDialog } ) => (
<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 visit, site-editor --> styles --> typography --> open font library modal --> upload font tab.

Note: Due to higher specificity of button class, it has changed the size of upload button to 40px which is not looking nice. Should we revert this change or add some style change by adding !importnant or higher specificity? Cc: @mirka @ciampo

Before After
upload-font-before Upload-font-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 raised this as a separate issue at #65333, but for the scope of this PR let's just add an !important to the height.

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 raised this as a separate issue at #65333, but for the scope of this PR let's just add an !important to the height.

Sure 👍.

@@ -87,8 +87,7 @@ function Palette( { name } ) {
{ window.__experimentalEnableColorRandomizer &&
themeColors?.length > 0 && (
<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 visit, site-editor --> styles --> color --> pallete, option for randmoize colors.

Before After
radomize-colors-before randomize-colors-after

@@ -162,8 +162,7 @@ function RevisionsButtons( {
aria-current={ isSelected }
>
<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 visit, site-editor --> styles --> revisions icon --> check for individual revisions button.

Note: This has some overridden styles, so no difference in before and after.

Before After
revisions-button-before revisions-button-after

@@ -234,8 +234,7 @@ export default function ShadowsEditPanel() {
>
<FlexItem>
<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 visit, site-editor --> styles --> shadows --> add shadow and rename it, a modal would be opened and check for cancel button.

Before After
shadow-rename-cancel-before shadow-rename-cancel-after

@@ -246,8 +245,7 @@ export default function ShadowsEditPanel() {
</FlexItem>
<FlexItem>
<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 visit, site-editor --> styles --> shadows --> add shadow and rename it, a modal would be opened and check for save button.

Before After
shadow-rename-save-before shadow-rename-save-after

@@ -381,8 +379,7 @@ function ShadowItem( { shadow, onChange, canRemove, onRemove } ) {
<HStack align="center" justify="flex-start" spacing={ 0 }>
<FlexItem style={ { flexGrow: 1 } }>
<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 visit, site-editor --> styles --> shadows --> add shadow. Open shadow and check for drop shadow button.

Note: It already has a 40px size as height is auto.

Before After
drop-showdow-button-before drop-shadow-button-aafter

@@ -394,8 +391,7 @@ function ShadowItem( { shadow, onChange, canRemove, onRemove } ) {
{ canRemove && (
<FlexItem>
<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 visit, site-editor --> styles --> shadows --> add shadow. Open shadow and check for drop shadow remove icon button.

Note: It already has a 40px size as height is auto.

Before After
drop-shadow-remove-before drop-shadow-remove-after

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

github-actions bot commented Sep 12, 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.

@akasunil akasunil added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 12, 2024
@mirka mirka requested a review from a team September 13, 2024 14:16
@hbhalodia
Copy link
Contributor Author

Hi @mirka, The requested changes are updated in the PR.
Thank You,

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, thanks 🚀

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