Action Menu #1152
Action Menu #1152
Conversation
…ce (indent) by non-selected items
|
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/6Pf5QKqrxDs9MszsjKXK38WbVK52 |
docs/content/ActionMenu.mdx
Outdated
| --- | ||
| 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 |
There was a problem hiding this comment.
Going to fill this out just a bit more when the API settles out 👍
src/stories/ActionMenu.stories.tsx
Outdated
| ComplexListStory.storyName = 'Complex List' | ||
|
|
||
| export function CustomTrigger(): JSX.Element { | ||
| const customAnchor = (props: any) => <Link {...props} /> |
There was a problem hiding this comment.
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.
VanAnderson
force-pushed
the
VanAnderson/action-menu
branch
from
5f93566 to
4b5023f
Compare
src/ActionMenu.tsx
Outdated
| renderAnchor?: (props: any) => JSX.Element | ||
| triggerContent?: React.ReactNode | ||
| renderItem?: (props: ItemProps) => JSX.Element | ||
| onActivate?: (props: ItemProps) => void |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Out of curiosity: Does (an ActionMenu-level) onActivate prop support any use case that couldn’t be handled by (Item-level) onClick handlers?
There was a problem hiding this comment.
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.
@types/get-random-values/index.d.ts
Outdated
| @@ -0,0 +1,4 @@ | |||
| declare module 'get-random-values' { | |||
There was a problem hiding this comment.
This makes for easier testing in node, rather than using window.crypto.getRandomValue
There was a problem hiding this comment.
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.getRandomValueis onerous, how many lines of code would it take to produce a utility function which provides a nicer API but useswindow.crypto.getRandomValueinternally?
There was a problem hiding this comment.
It's such a simple method, I went ahead and just replaced it with a util that will generate a random string 👍
VanAnderson
force-pushed
the
VanAnderson/action-menu
branch
from
f289a2a to
d6095b9
Compare
| ]} | ||
| /> | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Could you add an example that uses groupMetadata?
There was a problem hiding this comment.
done, just pulled from our ActionList docs on this one and kept them parallel.
|
Is this difference in spacing between the right and the left intentional?
|
Co-authored-by: Cole Bemis <colebemis@github.com>
Not intentional, just didn't notice it. I knocked out the right margin for the trailingVisualContainer, and I think this looks pretty even now:
|
Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com>
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