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 violations [Block Editor 5] #65361

Merged
merged 13 commits into from
Sep 25, 2024
Merged

Fix: Button Replace remaining 40px default size violations [Block Editor 5] #65361

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
246fe1b
Fix: inspector control tabs to use 40px default button size
parthVataliya16 Sep 16, 2024
9491543
Fix: inspector popover header to use 40px default button size
parthVataliya16 Sep 16, 2024
05ec17e
Fix: link control to use 40px default button size
parthVataliya16 Sep 16, 2024
78a6be7
Fix: list view to use 40px default button size
parthVataliya16 Sep 16, 2024
765e269
Fix: media placeholder to use 40px default button size
parthVataliya16 Sep 16, 2024
f680fc7
Add size attribute to inspector popover header and list view
parthVataliya16 Sep 17, 2024
24ed7ff
Revert the changes for submit button
parthVataliya16 Sep 17, 2024
92f1c20
Merge branch 'WordPress:trunk' into fix/issue-65018-5
PARTHVATALIYA Sep 24, 2024
3bc2b29
Replace button component
parthVataliya16 Sep 25, 2024
7c90785
Remove override CSS
parthVataliya16 Sep 25, 2024
f00a63a
Merge branch 'WordPress:trunk' into fix/issue-65018-5
PARTHVATALIYA Sep 25, 2024
e287a6d
Remove __next40pxDefaultSize prop
PARTHVATALIYA Sep 25, 2024
5086a1d
Remove __next40pxDefaultSize prop
PARTHVATALIYA Sep 25, 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
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/inspector-controls-tabs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ export default function InspectorControlsTabs( {
tabId={ tab.name }
render={
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
icon={
! showIconLabels ? tab.icon : undefined
}
Expand Down
6 changes: 2 additions & 4 deletions packages/block-editor/src/components/inspector-popover-header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ export default function InspectorPopoverHeader( {
<Spacer />
{ actions.map( ( { label, icon, onClick } ) => (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
PARTHVATALIYA marked this conversation as resolved.
Show resolved Hide resolved
key={ label }
className="block-editor-inspector-popover-header__action"
label={ label }
Expand All @@ -45,8 +44,7 @@ export default function InspectorPopoverHeader( {
) ) }
{ onClose && (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
PARTHVATALIYA marked this conversation as resolved.
Show resolved Hide resolved
className="block-editor-inspector-popover-header__action"
label={ __( 'Close' ) }
icon={ closeSmall }
Expand Down
9 changes: 3 additions & 6 deletions packages/block-editor/src/components/link-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,8 +408,7 @@ function LinkControl( {
{ ! showActions && (
<div className="block-editor-link-control__search-enter">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
onClick={ isDisabled ? noop : handleSubmit }
label={ __( 'Submit' ) }
icon={ keyboardReturn }
Expand Down Expand Up @@ -470,16 +469,14 @@ function LinkControl( {
className="block-editor-link-control__search-actions"
>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
variant="tertiary"
onClick={ handleCancel }
>
{ __( 'Cancel' ) }
</Button>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
variant="primary"
onClick={ isDisabled ? noop : handleSubmit }
className="block-editor-link-control__search-submit"
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/link-control/settings-drawer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ function LinkSettingsDrawer( { children, settingsOpen, setSettingsOpen } ) {
return (
<>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
className="block-editor-link-control__drawer-toggle"
aria-expanded={ settingsOpen }
onClick={ () => setSettingsOpen( ! settingsOpen ) }
Expand Down
3 changes: 1 addition & 2 deletions packages/block-editor/src/components/list-view/block-select-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ function ListViewBlockSelectButton(

return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
className={ clsx(
'block-editor-list-view-block-select-button',
className
Expand Down
21 changes: 7 additions & 14 deletions packages/block-editor/src/components/media-placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ const InsertFromURLPopover = ( {
value={ src }
/>
<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:
media-placeholder-1-before

After:
media-placeholder-1-after

Copy link
Contributor

Choose a reason for hiding this comment

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

@mirka will this be also affected by #65158 ?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. After #65158 lands, this media placeholder will also need to be updated to be a small button in the suffix slot, like in #65158 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

#65158 has landed, so after merging latest trunk into this branch, we can move the button to the suffix slot.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mirka I noticed that #65158 deals with URLInput, while this file is dealing with URLPopover, which doesn't seem to have a suffix. Should we just add the __next40pxDefaultSize prop to Button and use CSS to update the input height to 40px too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took another look and can confirm that URLPopover doesn't have a suffix — we can simply set __next40pxDefaultSize to true (like originally done by @PARTHVATALIYA ). The input will automatically match the new height, as per the screenshot above.

Copy link
Member

Choose a reason for hiding this comment

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

Actually we want to consolidate the UI pattern to have buttons in the suffix (see #64709 (comment)). And I noticed that this is a big enough change to warrant a separate code review, so I submitted a separate PR to deal with it (#65656).

In any case, not a blocker for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @mirka , I didn't initially understand that you were suggesting to use an InputControl. The work is going to be carried out in #65656, which means that there's nothing left to be done in this PR 🎉

className="block-editor-media-placeholder__url-input-submit-button"
icon={ keyboardReturn }
label={ __( 'Apply' ) }
Expand Down Expand Up @@ -87,8 +86,7 @@ const URLSelectionUI = ( { src, onChangeSrc, onSelectURL } ) => {
return (
<div className="block-editor-media-placeholder__url-input-container">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
className="block-editor-media-placeholder__button"
onClick={ openURLInput }
isPressed={ isURLInputVisible }
Expand Down Expand Up @@ -389,8 +387,7 @@ export function MediaPlaceholder( {
return (
onCancel && (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
className="block-editor-media-placeholder__cancel-button"
title={ __( 'Cancel' ) }
variant="link"
Expand Down Expand Up @@ -419,8 +416,7 @@ export function MediaPlaceholder( {
onToggleFeaturedImage && (
<div className="block-editor-media-placeholder__url-input-container">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
className="block-editor-media-placeholder__button"
onClick={ onToggleFeaturedImage }
variant="secondary"
Expand All @@ -436,8 +432,7 @@ export function MediaPlaceholder( {
const defaultButton = ( { open } ) => {
return (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
variant="secondary"
onClick={ () => {
open();
Expand Down Expand Up @@ -477,8 +472,7 @@ export function MediaPlaceholder( {
const content = (
<>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
variant="primary"
className={ clsx(
'block-editor-media-placeholder__button',
Expand Down Expand Up @@ -508,8 +502,7 @@ export function MediaPlaceholder( {
<FormFileUpload
render={ ( { openFileDialog } ) => (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
ciampo marked this conversation as resolved.
Show resolved Hide resolved
onClick={ openFileDialog }
variant="primary"
className={ clsx(
Expand Down
Loading