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

[docs-infra] Revamp the product switcher design #42603

Merged
merged 24 commits into from
Jun 13, 2024

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented Jun 10, 2024

Fix: #41183
Preview: https://deploy-preview-42603--material-ui.netlify.app/material-ui/getting-started/

Run-down of what this PR does:

  • Changed the implementation of the menu to fix the keyboard navigation problem: removing the Menu component and using Popper with Menu List instead
  • Removed "MUI Core"
  • Demphasized MUI X and added each individual component instead, with descriptions
  • Added the Toolpad logo (note that Joy and System don't have proper logos)

@danilo-leal danilo-leal added design This is about UI or UX design, please involve a designer scope: docs-infra Specific to the docs-infra product labels Jun 10, 2024
@danilo-leal danilo-leal self-assigned this Jun 10, 2024
@mui-bot
Copy link

mui-bot commented Jun 10, 2024

Netlify deploy preview

https://deploy-preview-42603--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 65da972

@danilo-leal danilo-leal changed the title [doca-infra] Revamp the product switcher design [docs-infra] Revamp the product switcher design Jun 10, 2024
@aarongarciah
Copy link
Member

Looks amazing. We need to fix the keyboard navigation.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Jun 11, 2024

We need to fix the keyboard navigation.

Yeah, so, that's technically fixed compared to the production version (you can tab through the options)(via the docs, not via the marketing pages yet), as I switched from using the Menu to using Popper + Menu List. However, I understand that to be inaccurate as in menus, we should be able to navigate via arrow keys instead of tabs (afaik). I had tried using Menu Items, still with the Menu, to see if I'd get all of that for free, but for some reason, that wasn't working properly.

@aarongarciah
Copy link
Member

I'm unable to navigate the options in the newly added menu. I tried tab and arrow keys with no luck 🤔

@danilo-leal
Copy link
Contributor Author

Uhm, I can do it by focusing on the docs button and then pressing enter.

tab.mp4

Pressing esc while the button is focused doesn't close the Popper, though (something else to fix, I guess).

@aarongarciah
Copy link
Member

Oh, I was trying from the home page 😅

Kapture.2024-06-11.at.12.57.55.mp4

@danilo-leal
Copy link
Contributor Author

Yeah, it's expected not to work on the marketing page's navbar—I haven't touched the code there given I'm considering removing the Docs button altogether (#42540).

@aarongarciah
Copy link
Member

Even better!

@danilo-leal danilo-leal marked this pull request as ready for review June 11, 2024 14:13
@danilo-leal danilo-leal requested review from aarongarciah, zanivan and a team June 11, 2024 14:13
Copy link
Contributor

@zanivan zanivan left a comment

Choose a reason for hiding this comment

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

It looks amazing! The logos and icons blend perfectly 🔥

I have a couple of remarks, more like questions:

  1. Should the navigation use arrows or tabs? IMO, arrows feel more natural, though I'm not sure what the best approach is. Whatever is decided, it's important to be sure that all menus follow the same interaction—both in the marketing and docs pages.
  2. I think we could use some animations and micro-interactions to spice things up a bit—I was thinking of something like this

@danilo-leal
Copy link
Contributor Author

Should the navigation use arrows or tabs? IMO, arrows feel more natural, though I'm not sure what the best approach is.

So, it's not even a matter of feeling, but rather accuracy to the the ARIA patterns specs. The prod implementation, using the Menu component, didn't have any keyboard navigation interaction working at all. I tried some different approaches on this PR (like using the Menu Item as the base component for the ProductItem custom component), to see if I'd get keyboard navigation for free from these Material UI components, but nope, it continued to not work (couldn't figure out yet exactly why). On the flip side, using the Popper, together with the Menu List (inspired by this), sort of fixed the keyboard nav issue (we can now focus on the menu button, open the popover, and tab through the menu items), but that isn't accurate to the menu pattern specs. That said, I'm unsure what's the best solution here: keep it as I've implemented here, or figure out why the Menu arrow keys keyboard nav is not properly working. I think having at least the Tab-based keyboard nav unlocked is an improvement we could put out. Next step could be to make it accurate by changing it to arrow keys instead. Unless someone has a clear idea on how to fix all of this and make it accurate to the specs right away!

I think we could use some animations and micro-interactions to spice things up a bit

Uhm... will think about it. 🤔

@colmtuite
Copy link
Contributor

It's missing a few things like

  • ESC to close
  • When you tab outside of it it should close.
  • Arrow key navigation
  • I just had a quick look but I think the popup is missing an aria role too, maybe not.

But in any case, neither of them are accessible currently, and this is an improvement. So I think it could be shipped.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Jun 11, 2024

ESC to close

It does close on esc if you're focused on a menu item; it doesn't otherwise because opening the popover doesn't automatically put focus on the first menu item (something that I was expecting to get for free from the Material components; there's something strange).

Arrow key navigation

That and tab navigation or that and then remove tab navigation (i.e., you can only navigate through via arrow keys)? That's the fundamental question I had with this PR.

I just had a quick look but I think the popup is missing an aria role too, maybe not.

Yeah, the popover doesn't have a role defined, but which one should it be? The ul tag inside the popover has role="menu" already. Should it be role="dialog"? Or should I put role="menu" in the popover instead? 🤔

@bharatkashyap
Copy link
Member

Looks fantastic @danilo-leal!

We're looking to divide the Toolpad section into Studio and Core in the upcoming month - I'm assuming we could do something like this when the time comes?

Screenshot 2024-06-12 at 2 20 41 PM

@danilo-leal
Copy link
Contributor Author

@bharatkashyap yeah, exactly—looks great!

@alexfauquette
Copy link
Member

The keyboard navigation is broken because you're breaking the contract with the MenuList children it's expecting an array of elements (one element per item to focus) and you provide a single Box component

Since the material-ui MenuList does not allow to manage nested component, I'm not able to do the structure <li><a/></li>

If I do so, the li components get the focus and so pressing Enter will not trigger the link.

It might be doable with base-ui since

Menu Item components don't have to be direct children of a Menu component. You can wrap them in any component needed to achieve the desired appearance.

https://mui.com/base-ui/react-menu/#wrapping-menu-items

Feel free to revert my commits

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Jun 12, 2024

Wouldn't use the current shape of the Base UI component now—maybe in the near future once the revamped Menu API is more ironed out & released. In any case, the only problem I see with your changes is what I said in the comment above: the Menu List is an ul tag and it's invalid HTML to have it with any other children other than li. The current markup is:

<ul role="menu">
   <a role="menuitem>
   <a role="menuitem>
   <a role="menuitem>
</ul>

I tried doing <MenuList component="div" ... /> and changing the forwarded type, but then TypeScript is just unhappy for a few reasons there. I'm not sure what the fix for this is yet. 🤔 Afraid it requires changing the whole thing 😖

@alexfauquette
Copy link
Member

TypeScript is just unhappy for a few reasons there. I'm not sure what the fix for this is yet. 🤔 Afraid it requires changing the whole thing 😖

TS aspect is fixed :)

@danilo-leal
Copy link
Contributor Author

@alexfauquette love it, thank you! Let me know if this is good to go 😃

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Both looks good 👍

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

This scales so much better for 4-6 X component 🥳
At some point we might have to introduce an "Other components" if we end up with Scheduler, Window splitter, File uploader, Rich Text Editor etc... but with this architecture I think we are good to go for the coming 18-24 months on X <3

@danilo-leal danilo-leal merged commit 0171518 into mui:next Jun 13, 2024
19 checks passed
@danilo-leal danilo-leal deleted the revamp-docs-product-switcher branch June 13, 2024 13:25
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Co-authored-by: alexandre <alex.fauquette@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs-infra] Tab navigation not working in MuiProductSelector
8 participants