feat(menu): add Menu.PlainButton as a minimally styled Menu button #1516

jinlee93 · 2023-02-28T06:02:14Z

EDS-845

Summary:

GST card requires enhancement of EDS Menu to allow custom button triggers
Changes Menu.Button from ClickableStyle to Button component
Introduces Menu.PlainButton, a minimally styled alternative for the opinionated Menu.Button
- Adds a story with FPO block
- Adds MenuWithIconButton recipe to show usage with an Icon

Test Plan:

Wrote automated tests
CI tests / new tests are not applicable
Manually tested my changes, but I want to keep the details secret
Manually tested my changes, and here are the details:
- Create an alpha publish and try out in edu-stack or traject as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.

codecov · 2023-02-28T06:02:57Z

Codecov Report

Merging #1516 (36a0910) into next (d37798c) will decrease coverage by 0.09%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             next    #1516      +/-   ##
==========================================
- Coverage   91.97%   91.88%   -0.09%     
==========================================
  Files         284      284              
  Lines        4300     4303       +3     
  Branches      793      793              
==========================================
- Hits         3955     3954       -1     
- Misses        320      324       +4     
  Partials       25       25

Impacted Files	Coverage Δ
src/components/Menu/Menu.stories.tsx	`66.66% <20.00%> (-10.26%)`	⬇️
src/components/Menu/Menu.tsx	`100.00% <100.00%> (ø)`
src/components/ClickableStyle/ClickableStyle.tsx	`87.09% <0.00%> (-9.68%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jinlee93 · 2023-02-28T06:03:56Z

adds buttonText prop to the main <Menu> which styles the button as previous, and opens up <Menu.Button> to be closer to <Button> component for more flexibility

Alternatively, we can check if typeof children === 'string' which would be non breaking but makes it more difficult if want to have custom button with just string

Another alternative would be to not provide further styling on top of the and just have recipes for styling and include chevron icon with the button text. However, mocks and docs for menu are currently opinionated as such so I am hesitant to remove them

github-actions · 2023-02-28T06:04:51Z

size-limit report 📦

Path	Size
components	121.14 KB (+0.04% 🔺)
styles	3.1 KB (0%)

src/components/Menu/Menu.tsx

jinlee93 · 2023-02-28T06:09:29Z

src/components/Menu/Menu.tsx

  return (
-    <HeadlessMenu.Button
-      as={ClickableStyle}


Originally was <ClickableStyle> but changed it to <Button> since the alternative (link) is not an appropriate trigger for a menu

I think this makes sense. The rationale here was that, since Headless gives you a <button>, this composed a styled button in the same way Button does internally. If we give it Button directly, looks like we get the same result. using ClickableStyle composes one less thing I guess

What things can be used as a menu trigger?

So far we have text in the mocks, and the vertical dots icon in GST

jinlee93 · 2023-02-28T06:09:53Z

src/components/Menu/Menu.tsx

-        />
-      </>
-    </HeadlessMenu.Button>
+    <HeadlessMenu.Button as={Button} status={status || 'neutral'} {...other} />


Kept as={Button} for accessibility features (such as target size) and EDS variants

Dang, Headless's Menu.Button isn't necessarily a button? 😬

It is but I was thinking keeping it EDS themed hence the EDS Button

src/components/Menu/Menu.stories.tsx

src/components/Menu/Menu.tsx

src/components/Menu/Menu.stories.tsx

src/components/Menu/Menu.tsx

ahuth · 2023-03-01T14:14:13Z

src/components/Menu/Menu.tsx

-        />
-      </>
-    </HeadlessMenu.Button>
+    <HeadlessMenu.Button as={Button} status={status || 'neutral'} {...other} />


Dang, Headless's Menu.Button isn't necessarily a button? 😬

anniehu4

nice, I really like this updated solution! clean and nicely leverages existing styling 👍 had 1 question about the status prop & wanted a look from @booc0mtaco but otherwise good to approve

src/components/Menu/Menu.tsx

src/components/Menu/Menu.test.tsx

anniehu4 · 2023-03-01T20:52:29Z

src/components/Menu/Menu.stories.tsx

+  },
+};
+
+export const DotsVerticalButton: StoryObj<MenuProps> = {


nit

Suggested change

export const DotsVerticalButton: StoryObj<MenuProps> = {

export const WithVerticalDotsButton: StoryObj<MenuProps> = {

@anniehu4 @jinlee93 we don't have a written convention for these but i have been using prefixes With if it demonstrates a composition, and When if the story exists to show some set of prop values.

Also, all With stories might actually be a signal that the story is actually a recipe in the format, "here's how you can combine ______ and _____ to get a _______"

WithVerticalDots does sound like a recipe, and could be redundant with the WithCustomButton story. Consider removing into a recipe?

@jinlee93 the custom ones using fpo stuff make sense per component, as they show where placeholders belong. We can add the recipe showing how to use the vertical dots as/with a custom button and keep both

Moved vertical dots story to its own recipe, since there already is a story using fpo block for button

makes sense to me! would be good to document that convention somewhere

src/components/Menu/Menu.stories.tsx

booc0mtaco

I did have an alternative suggestion for how we might compose custom buttons, mostly when this was a breaking change. This seems more straight-forward and extends default behavior!

booc0mtaco · 2023-03-01T21:17:14Z

src/components/Menu/Menu.stories.tsx

+  },
+};
+
+export const DotsVerticalButton: StoryObj<MenuProps> = {


@anniehu4 @jinlee93 we don't have a written convention for these but i have been using prefixes With if it demonstrates a composition, and When if the story exists to show some set of prop values.

Also, all With stories might actually be a signal that the story is actually a recipe in the format, "here's how you can combine ______ and _____ to get a _______"

src/components/Menu/Menu.test.tsx

src/components/Menu/Menu.tsx

booc0mtaco · 2023-03-01T21:29:05Z

src/components/Menu/Menu.tsx

  return (
-    <HeadlessMenu.Button
-      as={ClickableStyle}


I think this makes sense. The rationale here was that, since Headless gives you a <button>, this composed a styled button in the same way Button does internally. If we give it Button directly, looks like we get the same result. using ClickableStyle composes one less thing I guess

What things can be used as a menu trigger?

booc0mtaco · 2023-03-02T16:23:02Z

src/components/Menu/Menu.stories.tsx

+        <Menu.Button showExpandIcon={false} status="neutral" variant="icon">
+          <div className="fpo !py-0">Menu Button</div>
+        </Menu.Button>


I do believe there is an abstraction that can further simplify this, but I havent had the time to think thru it properly. As a hint tho, since we have control over what Menu.Button is, we could also specify additional .Button subcomponents that apply the desired props when used, so that consumers don't have to remember these prop values, class overrides, etc.

For example, something like:

<Menu.PlainButton> <div className="fpo">... or an icon, or an avatar component, etc.</div> </Menu.PlainButton>

In this case, whatever is in PlainButton (coming from a properly accessible design) takes over all the duties, and this essentially exposes the untreated HeadlessUI button.

This wouldn't block the current PR. If such an approach makes sense, it is additive and would mean we could later deprecate anything it would replace.

ooh interesting. I also just realized that the Button variants (especially "icon") don't work properly because the menu's built-in button styles are overriding it. Ideally we want to make this "plain" styling case really easy -- Andrew's suggestion would work, or something like <Menu.Button variant="plain">.

Using the EDS Button underneath still makes sense, but I'm now questioning if we should allow consumers to pass through all Button props ... may be better/less confusing for Menu to fully control the few cases

I'm liking the PlainButton api which just ships the Headless Menu Button. Not sure about the button variants as well. We can just go back to not accepting button props and if customization is wanted, consumers can use the PlainButton?

One other benefit of having a "plain" variant on Button/ClickableStyle is that it can manage implementing the focus ring. Ex:

<Button variant="plain"> <Avatar user={...} /> </Button>

would automatically allow a properly-spaced focus ring clamped around the shape of the children (or could have Tailwind utilities provided to change the wrapper shape to get a circle). PlainButton could allow as so the consumer can implement the entire button if we ship the headless wrapper. I'm not sure I follow the button prop bit, but we can chat thru that if it still is relevant

Think you commented this just as I pushed a commit with PlainButton haha. I do like the as prop.
The button prop was allowing MenuButton to accept Button props for further styling, but I removed that since that's possible with PlainButton now.

booc0mtaco · 2023-03-02T16:25:57Z

.storybook/recipes/MenuWithIconButton.stories.tsx

+        <Menu.Button
+          className="!border-transparent-white-0 !bg-transparent-white-0 hover:!bg-button-secondary-neutral-background-hover"
+          showExpandIcon={false}
+          variant="icon"
+        >
+          <Icon name="dots-vertical" purpose="informative" title="show more" />
+        </Menu.Button>


Related to that other comment, how might we create a sub-component that allows for this bit of code to be much briefer, and specify the proper prop values such that we avoid the boilerplate of lines 22-24

we should especially not need people to pass extra styling to get this case working 😬

Only needs the hover styling now with PlainButton

just noticed styling difference in recipe

jinlee93 · 2023-03-02T19:59:39Z

src/components/Menu/Menu.module.css

+  .menu__plain-button:focus {
+    @mixin focus;
+  }
+}


Just some light accessibility styling for the plain button. Can remove if we can assume if usage will always be accessible

@jinlee93 the designs should apply the accessible size, and specifying a minimum will be a visual hint that something is off if the input is too small.

We might also apply some centering in this case?

I think it's centered by default

Hesitant to add any more styling on top since TW classes don't override EDS styles in usage in edu-stack

.storybook/recipes/MenuWithIconButton.stories.tsx

src/components/Menu/Menu.module.css

anniehu4

nice!

booc0mtaco · 2023-03-03T18:41:07Z

@jinlee93 nudge to update the PR title if it's not accurate anymore :)

jinlee93 · 2023-03-03T18:42:49Z

@jinlee93 nudge to update the PR title if it's not accurate anymore :)

Ya will update summary too

## [11.0.0](v10.0.0...v11.0.0) (2023-03-17) ### ⚠ BREAKING CHANGES * add `indeterminate` prop to <Checkbox> that's separate from `checked` (#1520) ### Features * add `indeterminate` prop to <Checkbox> that's separate from `checked` ([#1520](#1520)) ([d8e2cc4](d8e2cc4)) * **LoadingIndicator:** extract and use SVG animation directly ([#1540](#1540)) ([6e315ea](6e315ea)) * **menu:** add Menu.PlainButton as a minimally styled Menu button ([#1516](#1516)) ([8268d8e](8268d8e)) ### Bug Fixes * actually use our shared prettier config ([c98ea51](c98ea51)) * **Avatar:** loosen props for avatar aria-label component ([#1544](#1544)) ([4ab9183](4ab9183)) * markdown story styling ([#1536](#1536)) ([89eba6b](89eba6b))

feat(menu)!: use buttonText prop and be more open with Menu button

0d9f452

jinlee93 requested a review from a team February 28, 2023 06:02

jinlee93 commented Feb 28, 2023

View reviewed changes

src/components/Menu/Menu.tsx Outdated Show resolved Hide resolved

jinlee93 commented Feb 28, 2023

View reviewed changes

src/components/Menu/Menu.stories.tsx Show resolved Hide resolved

jinlee93 commented Feb 28, 2023

View reviewed changes

src/components/Menu/Menu.tsx Outdated Show resolved Hide resolved

test(menubutton): add fpo story

f33f65e

ahuth reviewed Mar 1, 2023

View reviewed changes

refactor(menu): uncomplicate menu button and make unbreaking

a5120b4

anniehu4 reviewed Mar 1, 2023

View reviewed changes

jinlee93 changed the title ~~feat(menu)!: use buttonText prop and be more open with Menu button~~ feat(menu): use buttonText prop and be more open with Menu button Mar 1, 2023

booc0mtaco reviewed Mar 1, 2023

View reviewed changes

Jin Lee added 2 commits March 1, 2023 13:47

refactor(menubutton): rename class and fix status as neutral

06b93c4

refactor(menu): move vertical dots story to recipes

5f91cd5

booc0mtaco reviewed Mar 2, 2023

View reviewed changes

jinlee93 commented Mar 2, 2023

View reviewed changes

feat(menu): use plain button for custom styles

3987a5b

jinlee93 force-pushed the jlee/menuButtonMore branch from 4332d69 to 3987a5b Compare March 2, 2023 22:11

anniehu4 reviewed Mar 2, 2023

View reviewed changes

.storybook/recipes/MenuWithIconButton.stories.tsx Outdated Show resolved Hide resolved

anniehu4 reviewed Mar 2, 2023

View reviewed changes

src/components/Menu/Menu.module.css Show resolved Hide resolved

refactor(menu): add plain button styles and comment

ed4eaf4

anniehu4 approved these changes Mar 3, 2023

View reviewed changes

Merge branch 'next' into jlee/menuButtonMore

36a0910

jinlee93 changed the title ~~feat(menu): use buttonText prop and be more open with Menu button~~ feat(menu): add Menu.PlainButton for less opinionated Menu buttons Mar 3, 2023

jinlee93 changed the title ~~feat(menu): add Menu.PlainButton for less opinionated Menu buttons~~ feat(menu): add Menu.PlainButton as a minimally styled Menu button Mar 3, 2023

jinlee93 merged commit 8268d8e into next Mar 3, 2023

jinlee93 deleted the jlee/menuButtonMore branch March 3, 2023 18:55

booc0mtaco mentioned this pull request Mar 17, 2023

Release v11.0.0 #1549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(menu): add Menu.PlainButton as a minimally styled Menu button #1516

feat(menu): add Menu.PlainButton as a minimally styled Menu button #1516

jinlee93 commented Feb 28, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading

jinlee93 commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023 •

edited

Loading

jinlee93 Feb 28, 2023

booc0mtaco Mar 1, 2023

jinlee93 Mar 1, 2023

jinlee93 Feb 28, 2023

ahuth Mar 1, 2023

jinlee93 Mar 1, 2023

ahuth Mar 1, 2023

anniehu4 left a comment

anniehu4 Mar 1, 2023

booc0mtaco Mar 1, 2023

jinlee93 Mar 1, 2023 •

edited

Loading

booc0mtaco Mar 1, 2023

jinlee93 Mar 1, 2023

anniehu4 Mar 2, 2023

booc0mtaco left a comment

booc0mtaco Mar 1, 2023

booc0mtaco Mar 1, 2023

booc0mtaco Mar 2, 2023

anniehu4 Mar 2, 2023 •

edited

Loading

jinlee93 Mar 2, 2023

booc0mtaco Mar 2, 2023

jinlee93 Mar 2, 2023

booc0mtaco Mar 2, 2023

anniehu4 Mar 2, 2023

jinlee93 Mar 2, 2023

jinlee93 Mar 2, 2023 •

edited

Loading

booc0mtaco Mar 2, 2023

jinlee93 Mar 2, 2023

anniehu4 left a comment

booc0mtaco commented Mar 3, 2023

jinlee93 commented Mar 3, 2023

	export const DotsVerticalButton: StoryObj<MenuProps> = {
	export const WithVerticalDotsButton: StoryObj<MenuProps> = {

feat(menu): add Menu.PlainButton as a minimally styled Menu button #1516

feat(menu): add Menu.PlainButton as a minimally styled Menu button #1516

Conversation

jinlee93 commented Feb 28, 2023 • edited Loading

Summary:

Test Plan:

codecov bot commented Feb 28, 2023 • edited Loading

Codecov Report

jinlee93 commented Feb 28, 2023 • edited Loading

github-actions bot commented Feb 28, 2023 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anniehu4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinlee93 Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

booc0mtaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anniehu4 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinlee93 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anniehu4 left a comment

Choose a reason for hiding this comment

booc0mtaco commented Mar 3, 2023

jinlee93 commented Mar 3, 2023

jinlee93 commented Feb 28, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading

jinlee93 commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023 •

edited

Loading

jinlee93 Mar 1, 2023 •

edited

Loading

anniehu4 Mar 2, 2023 •

edited

Loading

jinlee93 Mar 2, 2023 •

edited

Loading