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 __next40pxDefaultSize for files in editor 4 #65140

Merged
merged 2 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.

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.

Copy link

github-actions bot commented Sep 8, 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: tyxla <tyxla@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.

Copy link

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 8, 2024
@AKSHAT2802 AKSHAT2802 changed the title Add __next40pxDefaultSize in button for editor 3 files Add __next40pxDefaultSize in button for editor 4 files Sep 8, 2024
@shail-mehta shail-mehta added the [Type] Code Quality Issues or PRs that relate to code quality label Sep 9, 2024
@AKSHAT2802 AKSHAT2802 changed the title Add __next40pxDefaultSize in button for editor 4 files Add __next40pxDefaultSize for files in editor 4 Sep 9, 2024
@@ -40,8 +40,7 @@ export default function TextEditor( { autoFocus = false } ) {
<div className="editor-text-editor__toolbar">
<h2>{ __( 'Editing code' ) }</h2>
<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
Exit code editor before exit code editor after

Copy link
Member

Choose a reason for hiding this comment

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

The button is looking good 👍

However, the "Editing code" heading is still using the old height (reflected in the heading line height), so it needs to be updated to match the new button height. This should work:

diff --git a/packages/editor/src/components/text-editor/style.scss b/packages/editor/src/components/text-editor/style.scss
index c8589fed6d..9b2748e08f 100644
--- a/packages/editor/src/components/text-editor/style.scss
+++ b/packages/editor/src/components/text-editor/style.scss
@@ -63,7 +63,7 @@
        }

        h2 {
-               line-height: $button-size;
+               line-height: $button-size-next-default-40px;
                margin: 0 auto 0 0;
                font-size: $default-font-size;
                color: $gray-900;

Here's how it will look before and after this change:

Before After
Screenshot 2024-09-11 at 15 14 39 Screenshot 2024-09-11 at 15 14 55

Copy link
Member

Choose a reason for hiding this comment

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

Good one. I added a todo item in #46741 to clean up these variable usages.

@@ -156,8 +156,7 @@ function StartModal( { slug, isCustom, onClose, postType } ) {
>
<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.

Before After
Screenshot 2024-09-09 at 6 39 40 PM Screenshot 2024-09-09 at 6 39 40 PM
Note: skip button variant set to `link`

@@ -124,8 +123,7 @@ const resetPost: Action< Post > = {
{ __( 'Cancel' ) }
</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.

Before After
Screenshot 2024-09-09 at 7 02 41 PM Screenshot 2024-09-09 at 7 02 12 PM

@@ -77,8 +76,7 @@ const trashPost: Action< PostWithPermissions > = {
{ __( 'Cancel' ) }
</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.

Before After
Screenshot 2024-09-09 at 7 06 05 PM Screenshot 2024-09-09 at 7 05 39 PM

@tyxla tyxla requested a review from a team September 10, 2024 13:13
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks for working on it @AKSHAT2802 🙌

This mostly looks good, the only thing we need to tweak is the TextEditor toolbar h2 line height - see my comment for more details.

Should be good to go once this is addressed 🚀

@@ -30,8 +30,7 @@ function TableOfContents(
contentClassName="table-of-contents__popover"
renderToggle={ ( { isOpen, onToggle } ) => (
<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.

This looks good, thanks 👍

Here are some details in case that can be useful:

TableOfContents is not in use since #44788.

When used somewhere, the button is an icon button, and it looks good with the new size:

Before After
Screenshot 2024-09-11 at 15 05 49 Screenshot 2024-09-11 at 15 06 55

No style overrides. However, the popover has some overrides (not something we should address in this PR of course), cc @WordPress/gutenberg-components:

.table-of-contents__popover.components-popover .components-popover__content {
min-width: 380px;
}
.components-popover.table-of-contents__popover {
z-index: z-index(".components-popover.table-of-contents__popover");
}
.table-of-contents__popover {
.components-popover__content {
padding: $grid-unit-20;
@include break-small {
max-height: calc(100vh - 120px);
overflow-y: auto;
}
}
hr {
margin: 10px -16px 0;
}
}

also cc @jorgefilipecosta should we deprecate TableOfContents?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @tyxla, yes I think we should deprecate it I will prepare a PR with the deprecation 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jorgefilipecosta, feel free to ping me for a review!

@@ -40,8 +40,7 @@ export default function TextEditor( { autoFocus = false } ) {
<div className="editor-text-editor__toolbar">
<h2>{ __( 'Editing code' ) }</h2>
<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.

The button is looking good 👍

However, the "Editing code" heading is still using the old height (reflected in the heading line height), so it needs to be updated to match the new button height. This should work:

diff --git a/packages/editor/src/components/text-editor/style.scss b/packages/editor/src/components/text-editor/style.scss
index c8589fed6d..9b2748e08f 100644
--- a/packages/editor/src/components/text-editor/style.scss
+++ b/packages/editor/src/components/text-editor/style.scss
@@ -63,7 +63,7 @@
        }

        h2 {
-               line-height: $button-size;
+               line-height: $button-size-next-default-40px;
                margin: 0 auto 0 0;
                font-size: $default-font-size;
                color: $gray-900;

Here's how it will look before and after this change:

Before After
Screenshot 2024-09-11 at 15 14 39 Screenshot 2024-09-11 at 15 14 55

@AKSHAT2802 AKSHAT2802 requested review from tyxla and mirka September 11, 2024 20:27
@AKSHAT2802
Copy link
Contributor Author

Thanks for working on it @AKSHAT2802 🙌

This mostly looks good, the only thing we need to tweak is the TextEditor toolbar h2 line height - see my comment for more details.

Should be good to go once this is addressed 🚀

Hii @tyxla @mirka

The suggested change to add line-height: $button-size-next-default-40px; for style.scss in text-editor is addressed.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

Thanks again @AKSHAT2802 🚀

@tyxla tyxla merged commit 394288f into WordPress:trunk Sep 12, 2024
62 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.

5 participants