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.

Sign up for GitHub

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

Jump to bottom

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Fix: Button: Replace remaining 40px default size violation [Block library 3] #65110

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff5e4d5
Fix the modal button for page list to use default 40px button size
hbhalodia Sep 6, 2024
cfd8c15
Fix convert page list to navigation links edit button to use 40px de…
hbhalodia Sep 6, 2024
3c7bd33
Fix enable comments form to use 40px default button size
hbhalodia Sep 6, 2024
87a37b7
Fix featured image block open media library button to use 40px defaul…
hbhalodia Sep 6, 2024
f5ddf47
Fix site-logo block to use 40px default button size
hbhalodia Sep 6, 2024
0d3029d
Fix social link apply link button to use 40px default size
hbhalodia Sep 6, 2024
0e297d9
Revert social link apply button size to todo state
hbhalodia Sep 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/block-library/src/page-list/convert-to-links-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,14 @@ export function ConvertToLinksModal( { onClick, onClose, disabled } ) {
</p>
<div className="wp-block-page-list-modal-buttons">
<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, while you add page-list block and try to convert page-list to navigationLinks

Before After
page-list-1-before page-list-1-after

variant="tertiary"
onClick={ onClose }
>
{ __( '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.

This can be seen, while you add page-list block and try to convert page-list to navigationLinks

Before After
page-list-1-before page-list-1-after

variant="primary"
accessibleWhenDisabled
disabled={ disabled }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/page-list/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,7 @@ export default function PageListEdit( {
<PanelBody title={ __( 'Edit this menu' ) }>
<p>{ convertDescription }</p>
<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, while you add page-list block and check the inspector panels, when convertToLinks option is true.

Before After
page-list-2-before page-list-2-after

variant="primary"
accessibleWhenDisabled
disabled={ ! hasResolvedPages }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/post-comments-form/form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ const CommentsForm = ( { postId, postType } ) => {
if ( 'closed' === commentStatus ) {
const 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.

This can be seen, while you add core/post-comments-form block, and you have comments disabled.

Before After
Comments-form-before Commentss-form-after

key="enableComments"
onClick={ () => setCommentStatus( 'open' ) }
variant="primary"
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/post-featured-image/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,7 @@ export default function PostFeaturedImageEdit( {
mediaLibraryButton={ ( { open } ) => {
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, while you add core/post-featured-image block.

Note: This has some styles that have higher specificity and specific to this upload button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After
featured-image-before featured-image-after

Copy link
Contributor

Choose a reason for hiding this comment

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

As @hbhalodia said, noting that the __next40pxDefaultSize prop here doesn't have an impact because of the style overrides applied to the placeholder and the button components in the post featured image block.

These style overrides should be removed, if possible — but it's not in the scope of this PR. We can keep changes as they are.

icon={ upload }
variant="primary"
label={ label }
Expand Down
6 changes: 2 additions & 4 deletions packages/block-library/src/site-logo/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,7 @@ export default function LogoEdit( {
render={ ( { open } ) => (
<div className="block-library-site-logo__inspector-upload-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.

This can be seen, while you add core/site-logo block, and check the inspector controls to upload the logo.

Note: This has some styles that have higher specificity and specific to this button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After
site-logo-before site-logo-after

Copy link
Contributor

Choose a reason for hiding this comment

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

We have style overrides in this case as well.

For the context of this PR, I think the current code changes are enough.

Still, I'd love to remove all of those overrides if possible, and just use a standard Button variant — although the situation is trickier than it looks. There are three possible UIs shown to the user:

Therefore, we'd need to make sure that we could offer a good, consistent UI in all three cases even without those overrides. Something we could do in a follow-up (cc @WordPress/gutenberg-design )/

Also, I noticed that the dropdown comes from the MediaReplaceFlow component. That dropdown is using a ToolbarButton as its trigger, showing that it was only meant to be rendered in a toolbar — while here it's being used in the editor sidebar (cc @WordPress/gutenberg-components @ntsekouras @youknowriad ).

onClick={ open }
variant="secondary"
>
Expand Down Expand Up @@ -674,8 +673,7 @@ export default function LogoEdit( {
mediaLibraryButton={ ( { open } ) => {
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, while you add core/site-logo block, check for upload button.

Note: This has some styles that have higher specificity and specific to this upload button on this block, hence before and after is same, but we can see our change is applied in inspector controls.

Before After
site-logo-2-before site-logo-2-after

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. There are again a lot of style overrides, which ideally shouldn't be there at all.

The code change here is good for the scope of this PR

icon={ upload }
variant="primary"
label={ __( 'Choose logo' ) }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-library/src/social-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,7 @@ const SocialLinkURLPopover = ( {
/>
</div>
<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, while you add core/social-link block, and add the link. You can check for the apply button.

Before After
social-link-before social-link-after

Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the URLInput to a 40px height too — @mirka , do you know if that's feasible as of now? Or should we keep the TODO comment and revisit later?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, this is kind of dependent on #64709, so I guess we can revert to TODO for now. I'll do something about it if the Button migrations get completed before the URLInput issue is addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I would update this to TODO state.

icon={ keyboardReturn }
label={ __( 'Apply' ) }
type="submit"
Expand Down
Loading