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

Generalize Menu Logic #4284

Merged
merged 6 commits into from May 8, 2023
Merged

Generalize Menu Logic #4284

merged 6 commits into from May 8, 2023

Conversation

tylerjbainbridge
Copy link
Contributor

@tylerjbainbridge tylerjbainbridge commented Apr 7, 2023

Context--
Typeaheads were created and then some parts of the code were reused/adapted for the AutoEmbed menus. This led to a lot of extra code in the LexicalTypeaheadMenuPlugin.tsx.

So in preparation for the upcoming LexicalContextMenuPlugin I've generalized the core menu logic into LexicalMenus.ts and separated the LexicalNodeMenuPlugin that's used by Embeds into it's own file.

Nothing functionally changed here. Things were just moved around/renamed to be more clear.

You can see this in action on the new-context-menu-v1 branch which will have PRs coming soon.

@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 Apr 7, 2023
@vercel
Copy link

vercel bot commented Apr 7, 2023

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 Apr 24, 2023 10:16pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 24, 2023 10:16pm

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.94 KB (0%) 539 ms (0%) 376 ms (+45.7% 🔺) 914 ms
packages/lexical-rich-text/dist/LexicalRichText.js 37.8 KB (0%) 757 ms (0%) 370 ms (-54.63% 🔽) 1.2 s
packages/lexical-plain-text/dist/LexicalPlainText.js 37.78 KB (0%) 756 ms (0%) 344 ms (-40.94% 🔽) 1.1 s

@zurfyx
Copy link
Member

zurfyx commented Apr 7, 2023

LGTM, but I don't think we can merge this just yet as removing modules is a breaking change. Do you want to keep the old modules for now or you want to prioritize the next Lexical version?

@vignesh-gupta
Copy link
Contributor

vignesh-gupta commented Apr 8, 2023

LGTM, but it might affect if anyone using old features.

@tylerjbainbridge tylerjbainbridge merged commit ddcf5be into main May 8, 2023
44 of 45 checks passed
@tylerjbainbridge tylerjbainbridge deleted the new-context-menu branch May 8, 2023 18:54
This was referenced May 23, 2023
@leolorenzoluis
Copy link

leolorenzoluis commented May 26, 2023

@tylerjbainbridge I don't know if you noticed but it broke the < .11 version because of this rename... what's the new guidance to use this API now? Are we expecting no backwards compatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants