[List v2]: Split lists on Enter in empty list items #39626
Closed
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
|
Size Change: +449 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Mar 22, 2022
youknowriad
reviewed
Mar 22, 2022
youknowriad
reviewed
Mar 22, 2022
youknowriad
reviewed
Mar 22, 2022
ntsekouras
force-pushed
the
try/listv2-split-lists-on-empty-with-enter
branch
from
59b0d10
to
65b6c46
Compare
ntsekouras
commented
Mar 22, 2022
| return ( | ||
| <li { ...innerBlocksProps }> | ||
| <RichText | ||
| ref={ useMergeRefs( [ useEnterRef ] ) } |
There was a problem hiding this comment.
I added useMergeRefs here because it will be needed or other custom handling in the follow ups about split/merge at least.
mcsf
reviewed
Mar 23, 2022
There was a problem hiding this comment.
This is looking good. You'll notice that all my comments are nit picks; that's because overall the code is 👌.
I do think there are little things here and there that can be done to make splitList easier to follow — some subtle naming changes and judicious inline comments would help.
The important thing here is the algorithm itself — not its implementation, but the choice of splitting method. Currently, any nested blocks that sit "below" the currently selected block (not just descendants, but also siblings, uncles, etc.) are "unwrapped" into top-level blocks in the after structure, which is not what the v1 list does.
To illustrate, take the following tree:
A.
1.
a.
<caret>
b.
2.
3.
B.
1.
2.
Pressing ENTER in List v1 yields:
A.
1.
a.
<caret>
b.
2.
3.
B.
1.
2.
and in v2:
A.
1.
a.
<caret>
b.
2.
3.
B.
1.
2.
Note: I don't think that v1 is necessarily the best possible method, but it's IMO still better than what we currently have here in v2. I'm open to other approaches. :)
Comment on lines
+66
to
+67
| const propsRef = useRef( props ); | ||
| propsRef.current = props; |
There was a problem hiding this comment.
This is just a nit pick, but this pair of statements caught my eye. I guess it's because ...ref = useRef( value ) carries a specific intention when I read code: it suggests that value is just the seed and not a continuously updating value — but the next line then refutes that assumption.
Curious, I searched our codebase for similar pairs:
ag --js -B1 '\.current = ' | grep -A1 useRefand it looks like we haven't decided between the two following idioms:
const ref = useRef();
ref.current = value;const ref = useRef( value );
ref.current = value;I have a personal preference for the former, because I think useRef() is clearer about the programmer's intentions. But, I reiterate, this is a huge nitpick and also very subjective. So, most of all, I'm curious on your opinion on this — without derailing the PR, of course.
Edit: it's also super interesting to read the names given to existing helper functions:
Comment on lines
+51
to
+54
| parent.innerBlocks = parent.innerBlocks.slice( | ||
| 0, | ||
| isLastMatch ? matchIndex : matchIndex + 1 | ||
| ); |
There was a problem hiding this comment.
Since we're doing in-place transformations, this might be a job more suited for Array#splice:
Suggested change
| parent.innerBlocks = parent.innerBlocks.slice( | |
| 0, | |
| isLastMatch ? matchIndex : matchIndex + 1 | |
| ); | |
| parent.innerBlocks.splice( isLastMatch ? matchIndex - 1 : matchIndex ); |
| clientId, | ||
| 'core/list' | ||
| )[ 0 ]; | ||
| const { beforeBlock, remainingBlocks } = splitList( |
There was a problem hiding this comment.
This one is also definitely a nit pick, just to keep some consistency with similar operations (e.g. useWritingFlow, useTabNav), both in terms of return type and naming:
Suggested change
| const { beforeBlock, remainingBlocks } = splitList( | |
| const [ before, after ] = splitList( |
| const matchParent = parent.innerBlocks[ matchIndex ]; | ||
| const blocksAfter = parent.innerBlocks.slice( matchIndex + 1 ); | ||
| if ( blocksAfter.length ) { | ||
| remainingBlocks.unshift( ...blocksAfter ); |
There was a problem hiding this comment.
I stopped for a minute to ponder the performance implications of Array#unshift (which has to jiggle elements around every time and thus is less performant than a series of #push followed by #reverse), but I don't think that realistically that will ever be a limiting factor. If someone has that many list items, the editor will probably lag somewhere else. :)
| } | ||
| replaceBlocks( | ||
| topParentListBlockClientId, | ||
| [ cloneBlock( beforeBlock ), ...extraBlocks ], |
There was a problem hiding this comment.
extraBlocks doesn't mean much, and this variable is capturing two different things (the new default block, and the optional tail list block), which suggests that we could split this:
const head = cloneBlock( before );
const middle = createBlock( getDefaultBlockName() );
const tail = after.length ? createBlock( 'core/list' , … ) : [];
replaceBlocks( topParentListBlockClientId, [ head, middle, ...tail ], 1 );(I'm not married to the variable names I used, but I do like the clarity of [head, middle, ...tail])
ntsekouras
force-pushed
the
try/listv2-split-lists-on-empty-with-enter
branch
from
65b6c46
to
8ce4f4e
Compare
I agree here! I tried implementing a different algorithm, but in general things can become very complex to cover all the situations. For example: Screen.Recording.2022-03-28.at.1.38.41.PM.movSo I think it's important to define the wanted algorithm before implementing it 😄 --cc @ellatrix @youknowriad @mtias |
|
I think pressing Screeny.Video.28.Mar.2022.at.13.50.44.mov |
|
Closing in favor of: #39858 |
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.
What?
Part of: #39519
In the experimental new
listblocks we should split lists on empty list items if we press enter.How?
Needs more work to handle the split more gracefully outside of current RichTexts
onSplitfunction.Testing Instructions
EnterScreenshots or screencast
Screen.Recording.2022-03-22.at.9.05.06.AM.mov