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

ActionMenu component #3081

Open
wants to merge 267 commits into
base: main
Choose a base branch
from
Open

ActionMenu component #3081

wants to merge 267 commits into from

Conversation

KenAJoh
Copy link
Collaborator

@KenAJoh KenAJoh commented Aug 9, 2024

Description

ActionMenu will act as new and improved replacement for our Dropdown, with functionality for

  • Sub-menus
  • Shortcuts
  • Checkboxes
  • Grouping
  • Radio-buttons
  • Separators
  • Full keyboard support
  • and lots of other small improvements ✨

TODO:

  • Update exports in index.ts
  • Check names in CSS
  • Update MenuItem with OverridableComponent
  • Move icon in item and checkbox to a prop

Examples and demonstrations for the website will be a new PR after this one is completed.

KenAJoh and others added 30 commits February 2, 2024 23:47
@navikt/core/react/src/overlays/action-menu/ActionMenu.tsx Outdated Show resolved Hide resolved
{children}
<Marker placement="left">
<Menu.ItemIndicator className="navds-action-menu__indicator">
<svg
Copy link
Contributor

Choose a reason for hiding this comment

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

There's something called SVG sprites. Don't know, but maybe this is a good use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check BEM:

  • "navds-action-menu--marker" (maybe "navds-action-menu__item--has-icon") (two places)
  • "navds-action-menu__checkbox" (maybe "navds-action-menu__item--checkbox")
  • "navds-action-menu__radio" (maybe "navds-action-menu__item--radio")
  • "navds-action-menu__sub-trigger" (maybe "navds-action-menu__item--sub-trigger")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated navds-action-menu--marker -> navds-action-menu--has-icon

Unsure about the others, since they are both an item (element) and a checkbox/radio/subtrigger (element) 🤔 If we follow "BEM" to the definition, this would make sense

navds-action-menu__item-checkbox
navds-action-menu__item-radio
navds-action-menu__item-sub-trigger

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say they are different variants of an item. In BEM terms, navds-action-menu__item is the Element and navds-action-menu__item--checkbox is a Modifier.

Looks like the BEM docs doesn't mention it, but I don't think it makes sense to have multiple "element-classes" on the same element. When reading the CSS you will get the impression that they are separate elements.

KenAJoh and others added 9 commits September 26, 2024 15:18
…3194)

* refactor: Remove extra CSS-classes, fixed center bug in safari

* refactor: Removed unused CSS
…d of pointermove (#3193)

* feat: ActionMenu now only opens submeny onClick

* memo: removed TODO

* feat: Closes all adjacent submenues when opening a new one

* refactor: Undo unwrapping of onDismiss

* refactor: Remove unused code in Menu.tsx

* feat: Handle submenu keys correctly

* feat: use native onClick for Trigger

* refactor: Remove un-needed disabled check on trigger

* bug: Reuse already used context

* Update @navikt/core/react/src/overlays/floating-menu/Menu.tsx

Co-authored-by: Halvor Haugan <[email protected]>

* bug: Rename open prop

* Memo: update comment

* feat: Use looser types for RovingFocus

* bug: Aboid auto-closing menu when opening with pointer

---------

Co-authored-by: Halvor Haugan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants