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

fix(lexical-playground): two issues with scrolling-related scenarios #2724

Merged
merged 18 commits into from
Aug 15, 2022

Conversation

LuciNyan
Copy link
Contributor

@LuciNyan LuciNyan commented Jul 28, 2022

Description

Adjusted the container when using createPortal so that the component and contentEditable are under the same parent element(anchor element) with position: relative (which also needs to be the scrolled element). The advantage of this is that we don't need to manually adjust the positioning when scrolling occurs.

Added the generic method setFloatingElemPosition to calculate positioning.

A more detailed description is here #2725.

The changes include:

  • CodeActionPlugin
  • FloatingLinkEditorPlugin
  • FloatingTextFormatToolbarPlugin
  • TableActionMenuPlugin
2022-07-28.14.21.35.mov
2022-08-06.15.53.11.mov

Close: #2725

@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 Jul 28, 2022
@vercel
Copy link

vercel bot commented Jul 28, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Aug 13, 2022 at 3:42AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Aug 13, 2022 at 3:42AM (UTC)

@LuciNyan LuciNyan changed the title Fix scroller fix(lexical-playground): two issues with scrolling-related scenarios Jul 28, 2022
@zurfyx
Copy link
Member

zurfyx commented Jul 29, 2022

I like your proposal! There's a few ways of solving the toolbar visibility problem. Admittedly, it's easier on full-page documents since the bar on top is a heavily tested pattern and can be implemented easily. For reference, we use a flex layout on our internal playground:

Screen Shot 2022-07-29 at 8 46 11 am

My greatest concern about your proposal is that we'll lose coverage on the reconciler bit that computes the scroll position. Currently, this playground is the only one that handles scroll at a window level and having a scrollable means that we wouldn't test that bit anymore (perhaps in no big deal?). For reference: https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalUtils.ts#L1131

I wonder what other folks think about this: @fantactuka @thegreatercurve @acywatson @tylerjbainbridge

@acywatson
Copy link
Contributor

I like your proposal! There's a few ways of solving the toolbar visibility problem. Admittedly, it's easier on full-page documents since the bar on top is a heavily tested pattern and can be implemented easily. For reference, we use a flex layout on our internal playground:

Screen Shot 2022-07-29 at 8 46 11 am

My greatest concern about your proposal is that we'll lose coverage on the reconciler bit that computes the scroll position. Currently, this playground is the only one that handles scroll at a window level and having a scrollable means that we wouldn't test that bit anymore (perhaps in no big deal?). For reference: https://github.com/facebook/lexical/blob/main/packages/lexical/src/LexicalUtils.ts#L1131

I wonder what other folks think about this: @fantactuka @thegreatercurve @acywatson @tylerjbainbridge

So you're point is that we would lose test coverage for the window-level scroll scenario, but I don't understand how that's related to the reconciler. Are you saying that we're indirectly testing Selection reconciliation through this scenario?

Generally, I'm ok with this approach. I think it makes sense for the way the editor works in the playground. I want to better understand the point @zurfyx is making though - I guess I could see long-form editors using window scroll + floating toolbars.

@LuciNyan LuciNyan marked this pull request as ready for review August 5, 2022 16:57
@LuciNyan
Copy link
Contributor Author

Hi @acywatson. It have been rebased.

@acywatson acywatson merged commit b7887a0 into facebook:main Aug 15, 2022
@LuciNyan LuciNyan deleted the fix-scroller branch August 16, 2022 02:00
thegreatercurve pushed a commit that referenced this pull request Nov 25, 2022
…2724)

* fix: scroller

* fix: code action when scroll

* fix: code action when scroll

* fix: scroller

* fix: text format floating

* fix: table menu

* chore: change name to anchorElem

* chore: add default value

* chore: rename

* chore: rename

* refactor: floating link editor

* feat: common calc floating position

* chore: move css

* chore: add useEffect deps

* chore: rename

* chore: update on scroll

* chore: resolve conflicts

* chore: re-trigger test
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.

Bug: two issues with scrolling-related scenarios
4 participants