-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Button: Add __next40pxDefaultSize
in block-editor 6
#65742
Conversation
Size Change: -251 B (-0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
@@ -28,8 +28,7 @@ export default function SkipToSelectedBlock() { | |||
|
|||
return selectedBlockClientId ? ( | |||
<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.
line-height: normal; | ||
box-shadow: 0 0 2px 2px rgba(0, 0, 0, 0.6); | ||
text-decoration: none; | ||
outline: none; | ||
z-index: z-index(".skip-to-selected-block"); |
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 removed some arbitrary and/or seemingly unnecessary style overrides. Maybe we can also remove the font size and weight, but left those as is for now.
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.
could we remove the background styles too?
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 can kind of see how someone would want a background color here, given that the button floats above a white background. I'd say a complete reconsideration of the remaining overrides (font size, weight, background) is a separate issue.
@@ -44,8 +44,7 @@ function ToolSelector( props, ref ) { | |||
<Dropdown | |||
renderToggle={ ( { isOpen, onToggle } ) => ( | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
size="compact" |
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.
@@ -38,8 +41,7 @@ class URLInputButton extends Component { | |||
return ( | |||
<div className="block-editor-url-input__button"> | |||
<Button | |||
// TODO: Switch to `true` (40px size) if possible | |||
__next40pxDefaultSize={ false } | |||
size="compact" |
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.
This isn't used in the app anymore, so I tested with a story.
diff --git a/packages/block-editor/src/components/url-input/stories/index.story.js b/packages/block-editor/src/components/url-input/stories/index.story.js
new file mode 100644
index 0000000000..68a5bc360b
--- /dev/null
+++ b/packages/block-editor/src/components/url-input/stories/index.story.js
@@ -0,0 +1,10 @@
+/**
+ * Internal dependencies
+ */
+import URLInputButton from '../button';
+
+export default { title: 'BlockEditor/URLInputButton' };
+
+export const Default = () => {
+ return <URLInputButton />;
+};
Before | After |
---|---|
I doubt anyone uses this anymore, since the styles are kind of broken/outdated already. I changed it to use the same suffix
UI pattern as the other similar URL controls.
@@ -540,8 +540,7 @@ class URLInput extends Component { | |||
> | |||
{ suggestions.map( ( suggestion, index ) => ( | |||
<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.
Insert a Paragraph block, select a string range and click the Link button in the block toolbar. I'm not sure what conditions are needed to reach this code naturally — it's not what you normally see in a URLInput in Gutenberg. To force the condition, you can comment out the block with the if ( isFunction( renderSuggestions ) )
conditional, a few lines above this.
Before | After |
---|---|
|
||
.components-button { | ||
flex-shrink: 0; | ||
width: $button-size; | ||
height: $button-size; | ||
} |
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.
This used to apply in the URLInputButton. No longer seems necessary.
|
||
return ( | ||
<fieldset className="block-editor-hooks__flex-layout-vertical-alignment-control"> |
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.
This FlexLayoutVerticalAlignmentControl
component is only used in this file, and only in the isToolbar={ true }
case. This ! isToolbar
condition is never reached. I also confirmed with a temporary story in Storybook that the styles/functionality in this code path is currently broken. Let's just remove the dead code.
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. |
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 the detailed review instructions! They made reviewing this PR a lot quicker.
🚀
Also — should we backport all PRs that are part of the 40px
migration? I don't think we backported all of them
line-height: normal; | ||
box-shadow: 0 0 2px 2px rgba(0, 0, 0, 0.6); | ||
text-decoration: none; | ||
outline: none; | ||
z-index: z-index(".skip-to-selected-block"); |
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.
could we remove the background styles too?
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.
Tests great. Great cleanup. Thanks @mirka 🚀
Oops. Added more tags. |
* Fix in Skip to selected block * Fix in Tool selector * Fix in URLInputButton * Fix in URLInput * Remove dead code in FlexLayoutVerticalAlignmentControl Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 716bc73 |
* Fix in Skip to selected block * Fix in Tool selector * Fix in URLInputButton * Fix in URLInput * Remove dead code in FlexLayoutVerticalAlignmentControl Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
* Fix in Skip to selected block * Fix in Tool selector * Fix in URLInputButton * Fix in URLInput * Remove dead code in FlexLayoutVerticalAlignmentControl Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
…ess#65742)" This reverts commit e9a991a.
* Fix in Skip to selected block * Fix in Tool selector * Fix in URLInputButton * Fix in URLInput * Remove dead code in FlexLayoutVerticalAlignmentControl Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
Part of #65018
What?
Adds the
__next40pxDefaultSize
prop to Buttons remaining in the block-editor package.