Skip to content

Button: Add __next40pxDefaultSize in dataviews, reusable-blocks, etc. #65715

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

Merged
mirka merged 6 commits into from
Sep 30, 2024
Merged

Button: Add __next40pxDefaultSize in dataviews, reusable-blocks, etc. #65715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
266d609
Fix in Dataforms layouts
mirka Sep 28, 2024
cdc56a7
Fix in Patterns header
mirka Sep 28, 2024
1509a20
Fix in Patterns import popover
mirka Sep 28, 2024
69affc9
Fix in nux
mirka Sep 28, 2024
848ff10
Fix in ReusableBlockConvertButton
mirka Sep 28, 2024
8b642e0
Update snapshot
mirka Sep 30, 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
4 changes: 1 addition & 3 deletions packages/dataviews/src/dataforms-layouts/panel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ function DropdownHeader( {
<Spacer />
{ onClose && (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
className="dataforms-layouts-panel__dropdown-header-action"
label={ __( 'Close' ) }
icon={ closeSmall }
onClick={ onClose }
size="small"
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Enable the "Quick Edit in DataViews" experiment. Go to the list of Pages in Site Editor and enable the Table view. Toggle the details panel to the right, and click on a field to show the edit popover.

No visual changes:

Quick edit popover

/>
) }
</HStack>
Expand Down
13 changes: 0 additions & 13 deletions packages/dataviews/src/dataforms-layouts/panel/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,3 @@
.dataforms-layouts-panel__dropdown-header {
margin-bottom: $grid-unit-20;
}

[class].dataforms-layouts-panel__dropdown-header-action {
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Removing this CSS because nothing else is using this class.

height: $icon-size;

&.has-icon {
min-width: $icon-size;
padding: 0;
}

&:not(.has-icon) {
text-decoration: underline;
}
}
4 changes: 2 additions & 2 deletions packages/list-reusable-blocks/src/components/import-dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ function ImportDropdown( { onUpload } ) {
contentClassName="list-reusable-blocks-import-dropdown__content"
renderToggle={ ( { isOpen, onToggle } ) => (
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
size="compact"
className="list-reusable-blocks-import-dropdown__button"
Comment on lines +20 to +21
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Go to http://localhost:8888/wp-admin/edit.php?post_type=wp_block.

Before After
Import from JSON button, before Import from JSON button, after

aria-expanded={ isOpen }
onClick={ onToggle }
variant="primary"
Expand Down
5 changes: 5 additions & 0 deletions packages/list-reusable-blocks/src/components/import-dropdown/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
.list-reusable-blocks-import-dropdown__content .components-popover__content {
padding: 10px;
}

[class].list-reusable-blocks-import-dropdown__button {
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

[class] selector added for more specificity.

Copy link
Member

@tyxla tyxla Sep 30, 2024

Choose a reason for hiding this comment

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

It's a bummer this is necessary, but it makes sense since the other button is outside our control.

// Todo: Make core button height and gutenberg button height equal.
height: 30px;
}
3 changes: 1 addition & 2 deletions packages/list-reusable-blocks/src/components/import-form/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,7 @@ function ImportForm( { instanceId, onUpload } ) {
</label>
<input id={ inputId } type="file" onChange={ onChangeFile } />
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Go to http://localhost:8888/wp-admin/edit.php?post_type=wp_block and click the Import from JSON button to show the popover.

Before After
Import button, before Import button, after

Copy link
Member

@tyxla tyxla Sep 30, 2024

Choose a reason for hiding this comment

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

Actually, does it make sense to use a compact button here?

I think the large 40px size feels odd with the rest of the buttons around:

Screenshot 2024-09-30 at 10 22 35

And here's how it would look with a size="compact":

Screenshot 2024-09-30 at 10 24 20

It's still 32px (vs 30px for the 2 other buttons), but 32px is much closer to 30px than 40px.

Thoughts?

Copy link
Member Author

@mirka mirka Sep 30, 2024

Choose a reason for hiding this comment

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

Mmm, it's a tough call! I agree the compact size looks much more harmonious in the current state, but also I believe the wp-admin pages are going to slowly migrate to the React componentry eventually. In that case, we will be adding tech debt by setting an explicit "compact" here, as that doesn't adhere to the design system guidelines. I think I slightly prefer the path of least resistance (i.e. default size), since everything is kinda wonky anyway? 😅

Copy link
Member

@tyxla tyxla Sep 30, 2024

Choose a reason for hiding this comment

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

You have a point about adding tech debt if we pick a different size.

I wish we could fix both the top buttons right here and right now, but it's a bigger can of worms.

type="submit"
isBusy={ isLoading }
accessibleWhenDisabled
Expand Down
1 change: 0 additions & 1 deletion packages/list-reusable-blocks/src/components/import-form/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

.list-reusable-blocks-import-form__button {
margin-top: 10px;
margin-bottom: 10px;
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Removed unnecessary margin bottom.

float: right;
}

Expand Down
4 changes: 0 additions & 4 deletions packages/list-reusable-blocks/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@
align-items: center;
position: relative;
top: -3px;
// Todo: Make core button height and gutenberg button height equal.
.components-button {
height: 26px;
}
Comment on lines -9 to -12
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

Moved to a custom class name in packages/list-reusable-blocks/src/components/import-dropdown/style.scss.

}

@include wordpress-admin-schemes();
6 changes: 2 additions & 4 deletions packages/nux/src/components/dot-tip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,15 @@ export function DotTip( {
<p>{ children }</p>
<p>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Member Author

@mirka mirka Sep 28, 2024

Choose a reason for hiding this comment

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

The nux package is not used anymore and has long been deprecated so we can't see an actual render, but here is an old screenshot:

Old screenshot of nux dot-tip

The link variant should remain the same height, and the "Disable tips" button is the close button in the top right.

variant="link"
onClick={ onDismiss }
>
{ hasNextTip ? __( 'See next tip' ) : __( 'Got it' ) }
</Button>
</p>
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
size="small"
Copy link
Member

@tyxla tyxla Sep 30, 2024

Choose a reason for hiding this comment

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

We'll also need to update the test snapshots here.

className="nux-dot-tip__disable"
icon={ close }
label={ __( 'Disable tips' ) }
Expand Down
4 changes: 2 additions & 2 deletions packages/nux/src/components/dot-tip/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ exports[`DotTip should render correctly 1`] = `
</p>
<p>
<button
class="components-button is-link"
class="components-button is-next-40px-default-size is-link"
type="button"
>
Got it
</button>
</p>
<button
aria-label="Disable tips"
class="components-button nux-dot-tip__disable has-icon"
class="components-button nux-dot-tip__disable is-small has-icon"
type="button"
>
<svg
Expand Down
6 changes: 2 additions & 4 deletions ...eusable-blocks/src/components/reusable-blocks-menu-items/reusable-block-convert-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,7 @@ export default function ReusableBlockConvertButton( {
/>
<HStack justify="right">
<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
Copy link
Member Author

@mirka mirka Sep 28, 2024

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, but we can infer that it's safe from the similar UI patterns like this:

Add new pattern popover

variant="tertiary"
onClick={ () => {
setIsModalOpen( false );
Expand All @@ -203,8 +202,7 @@ export default function ReusableBlockConvertButton( {
</Button>

<Button
// TODO: Switch to `true` (40px size) if possible
__next40pxDefaultSize={ false }
__next40pxDefaultSize
variant="primary"
type="submit"
>
Expand Down