Skip to content

Position menu immediately#7181

Merged
potatowagon merged 2 commits intomainfrom
menu-fix2
Feb 17, 2025
Merged

Position menu immediately#7181
potatowagon merged 2 commits intomainfrom
menu-fix2

Conversation

@zurfyx
Copy link
Copy Markdown
Member

@zurfyx zurfyx commented Feb 15, 2025

Credit to Cindy Zhang.

The currently menu position logic can interfere with other (third-party) components that attempt to position the component nicely within the screen. Immediate position makes it play better with these.

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 15, 2025

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 Feb 17, 2025 3:24am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 17, 2025 3:24am

@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 Feb 15, 2025
potatowagon
potatowagon previously approved these changes Feb 15, 2025
Comment on lines +599 to +607
// Append the context for the menu immediately
const containerDiv = anchorElementRef.current;
if (containerDiv != null) {
containerDiv.style.position = 'absolute';
if (parent != null) {
parent.append(containerDiv);
}
}

Copy link
Copy Markdown
Contributor

@potatowagon potatowagon Feb 17, 2025

Choose a reason for hiding this comment

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

causing context menu e2e tests to break, https://github.com/facebook/lexical/actions/runs/13342836119/job/37269678541?pr=7181

from the test failure videos & logs it looks like the test infra is unable to find the "Copy" option

waiting for locator('#typeahead-menu [role="option"] :text("Copy")') to be visible

im unable to repro that issue manually. think need to update the test selector

Copy link
Copy Markdown
Contributor

@potatowagon potatowagon Feb 17, 2025

Choose a reason for hiding this comment

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

tested manually the context menu seems fine

Screen.Recording.2025-02-17.at.10.15.57.AM.mov

so, likely a test issue where test needs update

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tests fixed in 8d52214

for some reason #typeahead-menu selector isnt working for the test, could be because changes to parent container re-assignments, anyway changing the selector to be a closer ancestor worked.

ran npm run test-e2e-chromium looks good locally

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.

3 participants