Skip to content

[Menubar] Create the Menubar component #1684

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

Open
wants to merge 81 commits into
base: master
Choose a base branch
from

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Apr 8, 2025

Preview: https://deploy-preview-1684--base-ui.netlify.app/react/components/menubar

Experiment: https://deploy-preview-1684--base-ui.netlify.app/experiments/menubar

Closes #1407

To do

  • modality
  • tests
  • docs and demos
  • vertical orientation
  • implementation using CompositeRoot
  • data-instant
  • Use the released version of Floating UI

Copy link

netlify bot commented Apr 8, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit f4f6563
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/680f30315048820008b28afe
😎 Deploy Preview https://deploy-preview-1684--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 31 to 44
const listNavigation = useListNavigation(floatingRootContext, {
enabled: !disabled,
listRef: itemDomElements,
activeIndex,
nested: false,
loop,
orientation,
rtl: direction === 'rtl',
onNavigate: (index) => {
if (index !== null) {
setActiveIndex(index);
}
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this has a lot of bugs, wouldn't it be better just to use Composite? This is missing the hover functionality if a menu is already open, but it avoids many issues by default

https://github.com/floating-ui/floating-ui/blob/master/packages/react/test/visual/components/Menubar.tsx

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll see if it's any better, thanks.

Comment on lines +101 to +102
returnFocus={parent.type === undefined}
initialFocus={parent.type === 'menu' ? -1 : 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Playing with this, it doesn't seem to make sense not to return focus in a web context. Radix and Ariakit return focus to the trigger. In the OS, menubar has different accessibility semantics most likely regarding where focus is placed, but inside a browser the focus should be restored, otherwise it just restarts at the beginning of the document and loses the context

@atomiks
Copy link
Contributor

atomiks commented Apr 22, 2025

Bug:

  1. Tab into menubar
  2. Hover over any item
  3. Should not open

@michaldudak
Copy link
Member Author

Bug:

Tab into menubar
Hover over any item
Should not open

This is by design to follow the pattern from MacOS menubar.

@atomiks
Copy link
Contributor

atomiks commented Apr 22, 2025

Hmm, it feels off to me. It's another thing Radix & Ariakit do differently to this, possibly making it different in a web context

cc: @colmtuite

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 22, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 22, 2025
@atomiks
Copy link
Contributor

atomiks commented Apr 23, 2025

The Tailwind CSS demo has no transitions. I think it'd be best if the transitions matched macOS: no fade in transition, but a fade out transition (no scaling, though)

@atomiks
Copy link
Contributor

atomiks commented Apr 23, 2025

On macOS, the open menu hides itself if you're still within the menubar bounds and not hovering over any trigger, but not if you go below the menubar in the "popups region" 🤔

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2025
@michaldudak
Copy link
Member Author

The Tailwind CSS demo has no transitions. I think it'd be best if the transitions matched macOS: no fade in transition, but a fade out transition (no scaling, though)

👍 I updated the demos.

On macOS, the open menu hides itself if you're still within the menubar bounds and not hovering over any trigger, but not if you go below the menubar in the "popups region" 🤔

Right. I don't think it's strictly necessary to replicate this, though.
Let's decide what to do with returning focus and hover behavior first, then we can consider what if this is worth implementing (@colmtuite, I'd love to hear your feedback).

@flaviendelangle
Copy link
Member

Screencast.2025-04-23.14.15.17.mp4

On Webstorm (Ubuntu), when I'm focusing an element in the menu and press "Escape", the focus moves back to the menu trigger.

With the Base UI component, is it something we want to have?
I saw that Arrow Left / Arrow Right allowed to move to the next / previous menu, but as a user my first reflex was to press "Escape" first which was a bad idea 😆

@atomiks
Copy link
Contributor

atomiks commented Apr 24, 2025

Bug with data-instant

  1. Tab to first menu trigger
  2. ArrowDown (doesn't happen with Enter)
  3. ArrowRight
  4. The first menu fades out but shouldn't transition

Bug with data-highlighted

  1. Tab to first menu trigger
  2. Enter (doesn't happen with ArrowDown)
  3. ArrowRight
  4. ArrowLeft
  5. data-highlighted is on the first menu item but shouldn't be

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 27, 2025
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 28, 2025
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.

[menu] Implement Menubar
5 participants