fix(lexical-list): remove list breaks if selection in empty #2672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
LuciNyan
requested review from
trueadm,
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
zurfyx
reviewed
Jul 21, 2022
There was a problem hiding this comment.
Thanks for taking a stab on this! Seems like the problem is deeper than a selection issue.
First of, note that .remove recovers the selection automatically when the selection is correctly placed. Suspect bad behavior when this does not happen.
Second, the behavior you described in the test is probably not what we're after, see how GDocs does it. It moves the list item node as a paragraph item.
Third, I would change a few things on the implementation:
- Remove items should work on the current item list node rather than trying to reorganize all the nodes
Why are we looking at all the top nodes? - I'm suspicious about the correctness of splice, probably worth revisiting selection logic there to make sure it's being recovered gracefully
- forEach is not recommended
Let me know if you want me to help with this
cc @acywatson
zurfyx
approved these changes
Jul 21, 2022
Thank you @zurfyx ! That's awesome! I learned a lot from your PR. You've solved a big potential problem. <3 |
karam-qaoud
pushed a commit
to karam-qaoud/lexical
that referenced
this pull request
Jul 26, 2022
…#2672) * fix(lexical-list): remove list breaks if selection in empty * chore: add a comment * chore: add test
karam-qaoud
added a commit
that referenced
this pull request
Jul 26, 2022
* Fixed getStyleObjectFromRawCSS function to work for unformatted css strings * Testing that handles unformatted css text * Testing that $getStyleObjectFromRawCss handles unformatted css * Added TableOfContents * Renamed TableOfContetnsPlugin file name and added flow file * Added TableOfContentsPlugin to config files and added styling * Fixed types * Added TableOfContentsList as a seperate module * Fixed type of tag from string to HeadingTagType * Table of contents updates as user scrolls * Wrapped plugin in a feature * Deleted package-lock.json * Fixed conditioanl rendering syntax * Removed extra parameter * package-lock * fix imports * Update packages/lexical-playground/src/plugins/TableOfContentsPlugin.tsx Co-authored-by: Gerard Rovira <zurfyx@users.noreply.github.com> * Added sticky styling and handled text overflow * Table of contents updates automtaically on scroll without observing all heading nodes * Update table correctly when headings are not visible but exist either up or down * Fix failing E2E * Changed isTableOfContentes to showTableOfContents in settings * Added useEffect to fix memory leak * Hoisted functions that don't use props * Renamed isTableOfContets to showTableOfContents * Changing selectedHeading by observing page top * resolved lint error * Refactored scroll up logic * Added comments * Added better css * Changed place of toc div to fix failing test * Fixed adjacent headings scrolling * Fixed adjacent headings bug * Renamed helper methods * Fixed test * Added dependency array to useEffect * Added TableOfContents to dependency array * Updated dependeny array in useEffect * Created e2e test file for table of contents plugin * Added scroll test * E2E test: Adding heading to editor adds them to table-of-contents * clean up * Refactored getEditorElement * Scrolling callback has better conditions * Table of contents is now covering all edge cases and doesn't freeze webpage * Solved page freezing * Added one more assert statment to second test * chore(lexical-playground): make directory clear (#2674) * Conditionally utilize `startTransition` if it's present (#2676) * Only utilize startTransition if it's available * Add type annotation * Run prettier * fix(lexical-list): remove list breaks if selection in empty (#2672) * fix(lexical-list): remove list breaks if selection in empty * chore: add a comment * chore: add test * Separate `@lexical/code` into more atomic modules (#2673) * separate code package into more atomic modules * remove utils * named exports * Fixed typo (#2678) * fix: path to icons (#2683) * Fix VALID_TWITTER_URL to allow underscores. (#2690) * fix(lexical-playground): LexicalTypeaheadMenuPlugin import (#2689) Use the correct import path that available in NPM package * Collapse and Expand DevTools Tree Nodes (#2679) * fix(playground): fix rendering Exclidraw (#2694) * Make includeHeaders a boolean (#2697) Changed type for includeHeaders parameter from string to boolean to match the type of the parameter from the $createTableNodeWithDimensions function. * Remove coverage reports (#2699) * fix: check if options are empty (#2701) * feat: Link node with target and rel (#2687) * OnChangePlugin ignoreInitialChange -> ignoreHistoryMergeTagChange (#2706) * OnChangePlugin ignoreInitialChange -> ignoreHistoryMergeTag * . * default to false because 0.4 Co-authored-by: Karam Qaoud <kqaoud@fb.com> Co-authored-by: Gerard Rovira <zurfyx@users.noreply.github.com> Co-authored-by: 子瞻 Luci <haru.lucinyan@gmail.com> Co-authored-by: Jack Hanford <jackhanford@gmail.com> Co-authored-by: John Flockton <thegreatercurve@users.noreply.github.com> Co-authored-by: SalvadorLekan <66782276+SalvadorLekan@users.noreply.github.com> Co-authored-by: Adithya Vardhan <imadithyavardhan@gmail.com> Co-authored-by: hiraoka <62982380+y-hiraoka@users.noreply.github.com> Co-authored-by: Elvin Dzhavadov <elvin.d@outlook.com> Co-authored-by: Will <will.gutierrez@gmail.com> Co-authored-by: Bryan <ImSingee@users.noreply.github.com> Co-authored-by: alinamusuroi <44519061+alinamusuroi@users.noreply.github.com> Co-authored-by: Andriy Chemerynskiy <andrzej.chem@gmail.com>
thegreatercurve
pushed a commit
that referenced
this pull request
Nov 25, 2022
* fix(lexical-list): remove list breaks if selection in empty * chore: add a comment * chore: add test
thegreatercurve
added a commit
that referenced
this pull request
Nov 25, 2022
* Fixed getStyleObjectFromRawCSS function to work for unformatted css strings * Testing that handles unformatted css text * Testing that $getStyleObjectFromRawCss handles unformatted css * Added TableOfContents * Renamed TableOfContetnsPlugin file name and added flow file * Added TableOfContentsPlugin to config files and added styling * Fixed types * Added TableOfContentsList as a seperate module * Fixed type of tag from string to HeadingTagType * Table of contents updates as user scrolls * Wrapped plugin in a feature * Deleted package-lock.json * Fixed conditioanl rendering syntax * Removed extra parameter * package-lock * fix imports * Update packages/lexical-playground/src/plugins/TableOfContentsPlugin.tsx Co-authored-by: Gerard Rovira <zurfyx@users.noreply.github.com> * Added sticky styling and handled text overflow * Table of contents updates automtaically on scroll without observing all heading nodes * Update table correctly when headings are not visible but exist either up or down * Fix failing E2E * Changed isTableOfContentes to showTableOfContents in settings * Added useEffect to fix memory leak * Hoisted functions that don't use props * Renamed isTableOfContets to showTableOfContents * Changing selectedHeading by observing page top * resolved lint error * Refactored scroll up logic * Added comments * Added better css * Changed place of toc div to fix failing test * Fixed adjacent headings scrolling * Fixed adjacent headings bug * Renamed helper methods * Fixed test * Added dependency array to useEffect * Added TableOfContents to dependency array * Updated dependeny array in useEffect * Created e2e test file for table of contents plugin * Added scroll test * E2E test: Adding heading to editor adds them to table-of-contents * clean up * Refactored getEditorElement * Scrolling callback has better conditions * Table of contents is now covering all edge cases and doesn't freeze webpage * Solved page freezing * Added one more assert statment to second test * chore(lexical-playground): make directory clear (#2674) * Conditionally utilize `startTransition` if it's present (#2676) * Only utilize startTransition if it's available * Add type annotation * Run prettier * fix(lexical-list): remove list breaks if selection in empty (#2672) * fix(lexical-list): remove list breaks if selection in empty * chore: add a comment * chore: add test * Separate `@lexical/code` into more atomic modules (#2673) * separate code package into more atomic modules * remove utils * named exports * Fixed typo (#2678) * fix: path to icons (#2683) * Fix VALID_TWITTER_URL to allow underscores. (#2690) * fix(lexical-playground): LexicalTypeaheadMenuPlugin import (#2689) Use the correct import path that available in NPM package * Collapse and Expand DevTools Tree Nodes (#2679) * fix(playground): fix rendering Exclidraw (#2694) * Make includeHeaders a boolean (#2697) Changed type for includeHeaders parameter from string to boolean to match the type of the parameter from the $createTableNodeWithDimensions function. * Remove coverage reports (#2699) * fix: check if options are empty (#2701) * feat: Link node with target and rel (#2687) * OnChangePlugin ignoreInitialChange -> ignoreHistoryMergeTagChange (#2706) * OnChangePlugin ignoreInitialChange -> ignoreHistoryMergeTag * . * default to false because 0.4 Co-authored-by: Karam Qaoud <kqaoud@fb.com> Co-authored-by: Gerard Rovira <zurfyx@users.noreply.github.com> Co-authored-by: 子瞻 Luci <haru.lucinyan@gmail.com> Co-authored-by: Jack Hanford <jackhanford@gmail.com> Co-authored-by: John Flockton <thegreatercurve@users.noreply.github.com> Co-authored-by: SalvadorLekan <66782276+SalvadorLekan@users.noreply.github.com> Co-authored-by: Adithya Vardhan <imadithyavardhan@gmail.com> Co-authored-by: hiraoka <62982380+y-hiraoka@users.noreply.github.com> Co-authored-by: Elvin Dzhavadov <elvin.d@outlook.com> Co-authored-by: Will <will.gutierrez@gmail.com> Co-authored-by: Bryan <ImSingee@users.noreply.github.com> Co-authored-by: alinamusuroi <44519061+alinamusuroi@users.noreply.github.com> Co-authored-by: Andriy Chemerynskiy <andrzej.chem@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When the anchor and focus of our selection fall on the textNode, we don't have to change the selection because the textNode will be appended to the newly generated paragraph.
When selection is in empty nested list item, selection is actually on the listItemNode. When the corresponding listItemNode is deleted and replaced by the newly generated paragraph, we should manually set the selection's focus and anchor to the newly generated paragraph.
closes #2640
2022-07-21.17.03.32.mov