Action Menu by VanAnderson · Pull Request #1152 · primer/react

VanAnderson · 2021-04-02T13:14:17Z

Putting this into review although there are still a few outstanding items that need some attention - it should be ready to start getting eyes on it.

This component uses the ActionList as a base to create an accessible and customizable dropdown menu.

Fixes#1069

…ot desired

…ce (indent) by non-selected items

…butes

changeset-bot · 2021-04-02T13:14:19Z

⚠️ No Changeset found

Latest commit: 91da62d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2021-04-02T13:14:20Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/6Pf5QKqrxDs9MszsjKXK38WbVK52
✅ Preview: https://primer-components-git-vananderson-action-menu-primer.vercel.app

VanAnderson · 2021-04-05T14:28:03Z

+---
+title: ActionMenu
+---
+
+An `ActionMenu` is a simple ActionList based component for define selectable menus.
+
+## Default example
+
+```jsx live
+<ActionMenu
+ buttonContent="Menu"
+ items={[
+ {text: 'New file', onClick: () => console.log('Do Something!')},
+ ActionMenu.Divider,
+ {text: 'Copy link'},
+ {text: 'Edit file'},
+ {text: 'Delete file', variant: 'danger'}
+ ]}
+/>
+```
+
+## Component props


Going to fill this out just a bit more when the API settles out 👍

VanAnderson · 2021-04-05T14:45:30Z

+ComplexListStory.storyName = 'Complex List'
+
+export function CustomTrigger(): JSX.Element {
+ const customAnchor = (props: any) => <Link {...props} />


Making a note on this - ideally we should just be able to pass Link directly to renderAnchor, but that doesn't seem to work in the way I thought it would.

…ustom Item renderer

VanAnderson · 2021-04-08T14:51:14Z

+ renderAnchor?: (props: any) => JSX.Element
+ triggerContent?: React.ReactNode
+ renderItem?: (props: ItemProps) => JSX.Element
+ onActivate?: (props: ItemProps) => void


onActivate is passed in and will trigger when a menu item is clicked or selected with the keyboard keys. This will pass ItemProps into the callback so the action can use the selected item to respond to the click or keyboard event.
This onActivate method might be more useful to users if we allowed a meta key of item props, where users could store actionable information about an item that could be used by onActivate to perform the items task/route to the proper place.

Out of curiosity: Does (an ActionMenu-level) onActivate prop support any use case that couldn’t be handled by (Item-level) onClick handlers?

I think part of what this is trying to make a bit nicer is handling both click and keyboard functionality in one interface. I think to do the same thing with item level handlers would end up being a bit messy.

VanAnderson · 2021-04-08T14:53:55Z

@@ -0,0 +1,4 @@
+declare module 'get-random-values' {


This makes for easier testing in node, rather than using window.crypto.getRandomValue

Historically, I believe we’ve avoided introducing external dependencies (especially ones without types) merely to improve the Primer developer experience (i.e. dependencies that do not also improve the experience of Primer consumers and the experience of their end-users).
Follow-up questions:
Do we have established guidance about when to add third-party dependencies @colebemis @T-Hugs? (e.g. specific criteria dependencies must meet, specific situations when we’d avoid third-party solutions, guidelines for choosing between alternatives)
If window.crypto.getRandomValue is onerous, how many lines of code would it take to produce a utility function which provides a nicer API but uses window.crypto.getRandomValue internally?

It's such a simple method, I went ahead and just replaced it with a util that will generate a random string 👍

VanAnderson · 2021-04-08T16:57:07Z

Not sure why vercel is failing - I don't have access to check the logs.

VanAnderson · 2021-04-08T17:07:37Z

+
+ // **
+ tabIndex?: number
+
+ ref?: React.RefObject<HTMLDivElement>


is there some sort of predefined type or interface we can use for these?

Both tabIndex and ref would be available if the ListPropsBase interface extends’d React.ComponentPropsWithRef<'div'>

smockle · 2021-04-08T17:39:20Z

 <div>{text}</div>
 {description && <DescriptionContainer>{description}</DescriptionContainer>}
 </StyledTextContainer>
+ {AuxilaryIcon && (


Nice work here! I love how this is consistent with the other conditional rendering (based on props) 🎊

smockle · 2021-04-08T17:44:10Z

+
 return (
- <StyledList {...props}>
+ <StyledList ref={containerRef as React.RefObject<HTMLDivElement>} {...props}>


Can we avoid the as here?

I had trouble making typescript happy here without it - though this pattern seems to be used other places in the code for this specific use case of the focusZone/focusTrap hooks.
I think there is going to be some changes incoming to how focusZones/focusTraps are implemented into ActionMenu/DropdownMenu/ActionList - I will keep an eye on how that work settles out. This might be something good to pair on that we can fix in a few places if we can find a better pattern.

… the Overlay open(rendered)/closed(not-rendered) state.

…delint

Co-authored-by: Cole Bemis <colebemis@github.com>

colebemis · 2021-04-16T20:14:49Z

+ ]}
+/>
+```
+


Could you add an example that uses groupMetadata?

done, just pulled from our ActionList docs on this one and kept them parallel.

colebemis · 2021-04-16T20:21:44Z

Is this difference in spacing between the right and the left intentional?

Co-authored-by: Cole Bemis <colebemis@github.com>

VanAnderson · 2021-04-16T20:37:34Z

Is this difference in spacing between the right and the left intentional?

Not intentional, just didn't notice it. I knocked out the right margin for the trailingVisualContainer, and I think this looks pretty even now:

colebemis

🚀 Nice work, @VanAnderson! ✨

dgreif

Looking great! Just a couple tweaks/questions 😄

Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com>

This reverts commit d6095b9.

smockle added 6 commits March 25, 2021 11:51

chore: Merge branch 'overlay' into dropdownmenu
2898c3c

chore: Merge branch 'main' into dropdownmenu
ae7841e

feat: Add DropdownMenu
f45dfdb

fix: Don’t hardcode a palette emoji in DropdownMenu; it’s typically n…
98f948e
…ot desired

feat: Enable deselection; draw checks by selected items and empty spa…
bb8e06d
…ce (indent) by non-selected items

feat: Add DropdownButton; begin adding required roles and other attri…
d9576c8
…butes

VanAnderson commented Apr 5, 2021
View reviewed changes

smockle mentioned this pull request Apr 5, 2021
ActionList #1078
Merged
7 tasks

chore: Merge branch 'main' into dropdownmenu
7bce122

VanAnderson force-pushed the VanAnderson/action-menu branch from 5f93566 to 4b5023fCompare April 7, 2021 14:37

smockle added 3 commits April 7, 2021 11:56

fix: Imported types, plus Storybook warnings, plus debug List-level c…
e3869a7
…ustom Item renderer

fix: Restore 'List'-level custom 'Item' rendering.
b812e73

chore: Merge branch 'main' into dropdownmenu
0cdaab2

VanAnderson marked this pull request as ready for review April 8, 2021 14:44

VanAnderson changed the title ~~[WIP] Action Menu~~Action MenuApr 8, 2021

VanAnderson commented Apr 8, 2021
View reviewed changes

smockle reviewed Apr 8, 2021
View reviewed changes

VanAnderson force-pushed the VanAnderson/action-menu branch from 0df23c9 to b1c11d7Compare April 9, 2021 20:24

VanAnderson requested a review from smockle April 9, 2021 20:33

smockleand others added 5 commits April 12, 2021 13:50

chore: Merge 'dropdown-with-zones' into 'dropdownmenu'
007ce3b

fix: keep focus on anchor when dropdown menu opens
16751fb

fix: Rename focus-related state and values to distinguish/dedupe from…
0029c37
… the Overlay open(rendered)/closed(not-rendered) state.

chore: Run Prettier
5edc447

fix: Add 'onDismiss' to the dependency list for 'onAnchorKeyDown' to …
f7bd529
…delint

dgreifand others added 4 commits April 16, 2021 10:45

refactor: optional chaining for item event handlers
75f9ed5
Co-authored-by: Cole Bemis <colebemis@github.com>

refactor: itemActivated -> handleSelection
33c4374

refactor: remove randomId in favor of uniqueId
8eabeee

Merge branch 'dropdownmenu' into VanAnderson/action-menu
807d6ae

dgreif mentioned this pull request Apr 16, 2021
Use github recommended lint in typescript files #1167
Merged

VanAnderson added 3 commits April 16, 2021 14:03

implement uniqueId for ActionMenu
9978e13

don't extend ActionList from div
9c8b666

registerPortalRoot root is optional
d6095b9

Base automatically changed from dropdownmenu to mainApril 16, 2021 19:37

VanAnderson force-pushed the VanAnderson/action-menu branch from f289a2a to d6095b9Compare April 16, 2021 19:59

VanAnderson added 2 commits April 16, 2021 15:01

Merge branch 'main' into VanAnderson/action-menu
9a14317

linter fix on Item
7cf5cc8

colebemis reviewed Apr 16, 2021
View reviewed changes

clean up Update src/ActionMenu.tsx click and keypress handlers
b90b79a
Co-authored-by: Cole Bemis <colebemis@github.com>

VanAnderson added 2 commits April 16, 2021 15:50

knock out right margin for trailingContainer
62f6fc4

add ActionMenu groupMetadata example
4a3afe2

colebemis approved these changes Apr 16, 2021
View reviewed changes

VanAnderson requested a review from dgreif April 16, 2021 21:17

dgreif suggested changes Apr 16, 2021
View reviewed changes

Comment threaddocs/content/ActionMenu.mdx Outdated
Comment threadsrc/ActionList/Item.tsx
Comment threadsrc/ActionMenu.tsx Outdated
Comment threadsrc/Portal/Portal.tsx Outdated
Comment threadsrc/index.ts
Comment threadsrc/stories/ActionMenu.stories.tsx Outdated

VanAndersonand others added 6 commits April 16, 2021 16:47

tweaks to Grouped ActionMenu examples.
b2bd62f
Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com>

useRenderForcingRef in ActionMenu
88ac566

Revert "registerPortalRoot root is optional"
7f6a811
This reverts commit d6095b9.

import useRenderForcingRef in ActionMenu
93cdab5

remove registerPortalRoot(undefined)
38bbce1

remove registerPortalRoot from imports
91da62d

dgreif approved these changes Apr 16, 2021
View reviewed changes

VanAnderson merged commit a975b5d into mainApr 16, 2021

VanAnderson deleted the VanAnderson/action-menu branch April 16, 2021 22:06

Uh oh!

Conversation

VanAnderson commented Apr 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-botBot commented Apr 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercelBot commented Apr 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smockleApr 8, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VanAndersonApr 8, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VanAnderson commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colebemis commented Apr 16, 2021

Uh oh!

VanAnderson commented Apr 16, 2021

Uh oh!

colebemis left a comment

Choose a reason for hiding this comment

Uh oh!

dgreif left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VanAnderson commented Apr 2, 2021•
edited
Loading

changeset-botBot commented Apr 2, 2021•
edited
Loading

vercelBot commented Apr 2, 2021•
edited
Loading

smockleApr 8, 2021•
edited
Loading

VanAndersonApr 8, 2021•
edited
Loading