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

DropdownMenu V2 tweaks #56041

Merged
merged 23 commits into from
Nov 23, 2023
Merged

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 10, 2023

What?

Closes #55933

This PR includes a series of tweaks to the experimental new version of DropdownMenu, based on the specs shared in #55933.

Main tweaks:

  • Removed group label component (not needed)
  • Added new subcomponents for menu item label and help text (mostly needed because of text truncation behavior)
  • Added auto-indentation of menu items based on the presence, in the same popover, of at least one item with a prefix
  • Tweaks to spacing, typography, colors, hovered/focused/disabled states...

Why?

The design spec brings a coherent look with the rest of the components library, and addresses a few complex scenarios when dealing with some combinations of menu items and their contents.

How?

The most complex bits in the implementation:

  • CSS grid and subgrid were used to implement the auto-indent functionality
  • The Label and Help text subcomponent use the Truncate component under the hood

Testing Instructions

Open the "DropdownMenu v2 ariakit" Storybook examples, make sure that the component follows the design specs

Screenshots or screencast

dropdown-menu-v2-tweaks.mp4

@ciampo ciampo force-pushed the feat/dropdown-menu-v2-ariakit-style-tweaks branch 2 times, most recently from 2d40fea to 8b66d75 Compare November 14, 2023 21:26
@ciampo ciampo self-assigned this Nov 14, 2023
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 14, 2023
Comment on lines +28 to +31
// TODO:
// - those values are different from saved variables?
// - should bring this into the config, and make themeable
// - border color and divider color are different?
Copy link
Contributor Author

@ciampo ciampo Nov 14, 2023

Choose a reason for hiding this comment

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

Flagging this for us to discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

border color and divider color are different?

Yup that's fine.

should bring this into the config, and make themeable

I'm not sure what is entailed, but the components should eventually be themable using the new color scales:

theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clarifying.

There is one more question from the list above that I wanted to clarify — from the design spec, it seems that the border color is gray 300, and the divider is gray 200.

While these values seem to align with the inline comments where the grays are defined, I noticed that we have some variables specific to borders that are not using the same colors — I guess it would be good if you could review where that are being used, and potentially give feedback around whether we can tweak those variables to use gray 300 and 200.

Copy link
Contributor

Choose a reason for hiding this comment

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

The second group of vars seem to be applied to the experimental ToggleGroupControl. They make sense in the context of an interactive element like a form input where there are stricter a11y requirements for contrast. Visual elements (like the menu container) can use the lighter borders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, and likely something that we should codify when working on the wider theme / design system

Comment on lines +78 to +79
display: grid;
grid-template-columns: minmax( 0, max-content ) 1fr;
Copy link
Contributor Author

@ciampo ciampo Nov 14, 2023

Choose a reason for hiding this comment

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

This is where the main grid layout is defined.

The first column is for the prefix (and can collapse to 0 width if no prefixes are rendered within the same dropdownmenu popover).

The second column take all remaining available space, and is meant to host the wrapper element containing label, help text, and suffix.

{ suffix && (
<Styled.ItemSuffixWrapper>{ suffix }</Styled.ItemSuffixWrapper>
) }
<Styled.ItemPrefixWrapper>{ prefix }</Styled.ItemPrefixWrapper>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prefix wrapper is always rendered (even when empty) in order to correctly render in the first column of the grid layout.

* from the parent layout (ie. subgrid).
*/
display: grid;
grid-template-columns: subgrid;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CSS subgrid enables us to define a grid layout at the menu level, and "propagate" it down all the way to the menu item children.

*/
grid-column: 2;

display: flex;
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 wrapper occupies the second "column" of the grid layout, and is a flexbox container. This mixed grid + flexbox setup enables menu items to align their left indentation based on the presence of at least one prefix, but keeps independent around the right-side alignment


export const DropdownMenuGroup = styled( Ariakit.MenuGroup )`
/* Ignore this element when calculating the layout. Useful for subgrid */
display: contents;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display contents allows us to ignore this element when calculating the layout, and look at its children directly

aria-hidden="true"
icon={ chevronRightSmall }
size={ 24 }
preserveAspectRatio="xMidYMid slice"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

preserveAspectRatio is used to resize the width of the icon while preserving its real size (ie. slicing the SVG viewport)

<ExampleSlotFill.Slot
fillProps={ fillProps }
bubblesVirtually
style={ { display: 'contents' } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding display: contents to the Slot element is necessary, otherwise the slotfill example breaks the grid layout.

@ciampo ciampo marked this pull request as ready for review November 14, 2023 22:00
@ciampo ciampo requested a review from ajitbohra as a code owner November 14, 2023 22:00
@ciampo ciampo requested review from jameskoster, richtabor and a team November 14, 2023 22:07

export const DropdownMenuItemLabel = styled( Truncate )`
font-size: ${ font( 'default.fontSize' ) };
line-height: 20px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have any presets for line-height? Or are these values ad-hoc for this component?

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 text styles in Figma, so these values aren't ad hoc. I don't know how well those styles are reflected in the codebase though. All I'm aware of is https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/text/styles/text-mixins.native.js.

Typography is due a bit of a revamp for the admin work (#53322).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, then I guess we can leave this as-is and make sure we revisit after we have some better common typography styles.

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

This all looks good and tests well to me - the only deviation from the spec that jumped out to me was that disabled items aren't getting a not-allowed cursor when hovered, but that may be something you're planning to address separately.

@andrewhayward
Copy link
Contributor

andrewhayward commented Nov 20, 2023

I don't know if this is the right place for this feedback, but for want of anywhere else...

Scroll margins

When there isn't enough space to render the whole menu, it gets a scrollbar, which is fine, but the menu items need a scroll margin to make sure they're fully visible.

A menu with a scrollbar. The first menu item has a focus ring, cut off by the scroll position.
--- a/packages/components/src/dropdown-menu-v2-ariakit/styles.ts
+++ b/packages/components/src/dropdown-menu-v2-ariakit/styles.ts
@@ -188,16 +188,21 @@ const baseItem = css`
        }
 `;

-export const DropdownMenuItem = styled( Ariakit.MenuItem )`
+const menuItem = css`
        ${ baseItem };
+       scroll-margin: ${ CONTENT_WRAPPER_PADDING };
+`;
+
+export const DropdownMenuItem = styled( Ariakit.MenuItem )`
+       ${ menuItem };
 `;

 export const DropdownMenuCheckboxItem = styled( Ariakit.MenuItemCheckbox )`
-       ${ baseItem };
+       ${ menuItem };
 `;

 export const DropdownMenuRadioItem = styled( Ariakit.MenuItemRadio )`
-       ${ baseItem };
+       ${ menuItem };
 `;

 export const ItemPrefixWrapper = styled.span`

Disabled menu items

Every menu item should really be focusable, which isn't the default with Ariakit. We should set accessibleWhenDisabled as standard, so that consumers have to explicitly override it.

A menu with a disabled menu item showing a focus ring.
index aac8a8844f..a318d295a8 100644
--- a/packages/components/src/dropdown-menu-v2-ariakit/index.tsx
+++ b/packages/components/src/dropdown-menu-v2-ariakit/index.tsx
@@ -53,6 +53,7 @@ export const DropdownMenuItem = forwardRef<
        return (
                <Styled.DropdownMenuItem
                        ref={ ref }
+                       accessibleWhenDisabled
                        { ...props }
                        hideOnClick={ hideOnClick }
                        store={ dropdownMenuContext?.store }
@@ -86,6 +87,7 @@ export const DropdownMenuCheckboxItem = forwardRef<
        return (
                <Styled.DropdownMenuCheckboxItem
                        ref={ ref }
+                       accessibleWhenDisabled
                        { ...props }
                        hideOnClick={ hideOnClick }
                        store={ dropdownMenuContext?.store }
@@ -130,6 +132,7 @@ export const DropdownMenuRadioItem = forwardRef<
        return (
                <Styled.DropdownMenuRadioItem
                        ref={ ref }
+                       accessibleWhenDisabled
                        { ...props }
                        hideOnClick={ hideOnClick }
                        store={ dropdownMenuContext?.store }

@ciampo
Copy link
Contributor Author

ciampo commented Nov 22, 2023

Thank you @chad1008 and @andrewhayward for your reviews!

I've pushed 310eb12, da57e59 and 264b4fc which should address your feedback.

@jameskoster , would you be able to take a look at this PR? I also left a couple of comments (one, two) highlighting a few points that I wanted to discuss.

@ciampo ciampo requested a review from chad1008 November 22, 2023 10:31
Copy link

Flaky tests detected in 264b4fc.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6955817559
📝 Reported issues:

@ciampo ciampo requested a review from a team November 23, 2023 12:57
@ciampo ciampo force-pushed the feat/dropdown-menu-v2-ariakit-style-tweaks branch from c1ce534 to 5e02c8c Compare November 23, 2023 12:58
@ciampo
Copy link
Contributor Author

ciampo commented Nov 23, 2023

@jameskoster and @WordPress/gutenberg-components , I believe that this PR is ready for a last round of review. It would be great if we could merge this PR soon, since it unblocks a bunch of other dropdownmenu-related work.

Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

There's one tiny detail to fix. We need to update the offset position values accounting for the new padding. Notice how the flyouts aren't lining up nicely now:

Screenshot 2023-11-23 at 14 18 59

@ciampo
Copy link
Contributor Author

ciampo commented Nov 23, 2023

@jameskoster thank you for the detailed screenshot! Done

@ciampo ciampo requested a review from jameskoster November 23, 2023 14:38
Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

I think we're looking good. There might be some minor visual tweaks down the road as we make more practical use of the component, but those are easy to revisit in smaller PRs as required.

@ciampo ciampo enabled auto-merge (squash) November 23, 2023 16:41
@ciampo ciampo merged commit b55f86f into trunk Nov 23, 2023
51 checks passed
@ciampo ciampo deleted the feat/dropdown-menu-v2-ariakit-style-tweaks branch November 23, 2023 16:51
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DropdownMenu v2: Design tweaks
5 participants