8000 FloatingUI Context Menu by ivailop7 · Pull Request #7509 · facebook/lexical · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

FloatingUI Context Menu #7509

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

FloatingUI Context Menu #7509

wants to merge 9 commits into from
+500 −175

Conversation

ivailop7
Copy link
Collaborator
@ivailop7 ivailop7 commented May 5, 2025 •
edited 8000
Loading

A complete rewrite of the ContextMenu using FloatingUI without using the current LexicalMenu component. This makes the migration of the other typeahead menus to FloatingUI easier.

This is a breaking change. The migration effort should be minimal as I kept the playground top-level interface as close as possible.

This PR introduces the following improvements:

  • Converts the 'li' items to 'buttons' (This leads to better overall accessibility)
  • The menu has the correct aria role
  • Correct positioning at edges and when page is scrolled
  • Typeahead accessibility (when context menu is open, pressing a keyboard character moves the selection)
  • Keyboard navigation (standard arrows navigation)
  • Better logic for conditional items rendering (display extra items when a node-of-type)
  • When displayed locks the page scroll and preserves focus correctly consistent with other editors
  • Exposes the renderers for the ContextMenu and ContextMenuItem to be overriden and styled as desired
  • Support for disabled items and custom styling
  • Support for a separator and custom styling

@vercel Vercel
Copy link
vercel bot commented May 5, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2025 11:05pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 11, 2025 11:05pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 5, 2025
@ivailop7
Copy link
Collaborator Author
ivailop7 commented May 5, 2025

@etrepum This is ready to skim over. Playground is produced fine. I'll fix the types tomorrow. Most of the code is canonical as per the FloatingUI example for a context menu.

I had to make some creative choices for the conditional rendering and the editor.read for the underlying node, since the examples use pre-defined children to the main component and wanted to keep the main render components exposed to be overridden.

Welcoming interface comments.
8000

@etrepum
Copy link
Collaborator
etrepum commented May 6, 2025

The things I'd do differently in the interface:

  • Rename showOn and onSelect to $showOn and $onSelect to make it clear that they are called with editor context (I think the onSelect implementation is currently broken in this refactor for this reason, the delete node code depends on editor context)
  • Unify options and conditionalOptions, there doesn't really seem to be a reason to have them be separate and by having a single array you can control the exact order

As far as the implementation goes, just some things I'd simplify:

  • I don't think it's useful to bind showOn and onSelect to this because the ContextMenuOption class isn't exported and there's not any particularly useful state in there. If they wanted it bound to something they would probably prefer the object that it was defined in (per the constructor) and the caller could do that binding themselves if they really wanted to.
  • I generally don't like cloneElement, especially when not used to facilitate an eDSL where the user is specifying the children. This code would be simpler if the items array was props and then you can do the <ContextMenuItem {...getItemProps({ ...props, onClick() { props.onClick(); setIsOpen(false); }, /* … */ } })} /> instead of going through the trouble to create the JSX to turn it back into props to create the JSX again.

@ivailop7
Copy link
Collaborator Author
ivailop7 commented May 6, 2025

The things I'd do differently in the interface:

  • Rename showOn and onSelect to $showOn and $onSelect to make it clear that they are called with editor context (I think the onSelect implementation is currently broken in this refactor for this reason, the delete node code depends on editor context)

  • Unify options and conditionalOptions, there doesn't really seem to be a reason to have them be separate and by having a single array you can control the exact order

As far as the implementation goes, just some things I'd simplify:

  • I don't think it's useful to bind showOn and onSelect to this because the ContextMenuOption class isn't exported and there's not any particularly useful state in there. If they wanted it bound to something they would probably prefer the object that it was defined in (per the constructor) and the caller could do that binding themselves if they really wanted to.

  • I generally don't like cloneElement, especially when not used to facilitate an eDSL where the user is specifying the children. This code would be simpler if the items array was props and then you can do the <ContextMenuItem {...getItemProps({ ...props, onClick() { props.onClick(); setIsOpen(false); }, /* … */ } })} /> instead of going through the trouble to create the JSX to turn it back into props to create the JSX again.

Great! Thanks! I'll fix those up. The reason I split the conditional and default options was, that conditional options was intended as on object offering multiple items per 1 rule eval, vs 1 rule for 1 option, but forgot to do it.
I'll also think of moving the Item component to the main menu, will help me when I unify some of the internals with the other menus for later.

One last question I have is, should I add a new plugin in the react package and export ContextMenu2 and make this opt-in and make the breaking change later when we have all menus migrated vs now.

@etrepum
Copy link
Collaborator
etrepum commented May 6, 2025

Maybe call it NodeContextMenu and deprecate the other one?

@ivailop7 ivailop7 added the extended-tests Run extended e2e tests on a PR label May 11, 2025
@ivailop7 ivailop7 marked this pull request as ready for review May 11, 2025 22:49
@ivailop7 ivailop7 marked this pull request as draft May 11, 2025 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

@Fetz Fetz Awaiting requested review from Fetz Fetz is a code owner

@Sahejkm Sahejkm Awaiting requested review from Sahejkm Sahejkm is a code owner

@potatowagon potatowagon Awaiting requested review from potatowagon potatowagon is a code owner

@lilshady lilshady Awaiting requested review from lilshady lilshady is a code owner

@takuyakanbr takuyakanbr Awaiting requested review from takuyakanbr takuyakanbr is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivailop7 @etrepum @facebook-github-bot
0