Navigation Link block: add RichText split-at-end/merge/remove behavior #21764
Conversation
ZebulanStanphill
added
[Type] Enhancement
|
Size Change: +140 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
fix/21743
branch
2 times, most recently
from
5e17fbc to
9884040
Compare
|
I thought this would be a simple fix, but I've run into an issue I didn't expect. After pressing backspace and removing the block, focus doesn't move to the previous Navigation Link like you would expect. No idea why. 😕 |
ZebulanStanphill
force-pushed
the
fix/21743
branch
from
9884040 to
07fd2f6
Compare
|
@ZebulanStanphill From what I can gather, |
ZebulanStanphill
force-pushed
the
fix/21743
branch
4 times, most recently
from
2002629 to
bb599bb
Compare
|
@talldan Thanks; I added merge functionality and now the cursor is moving to the previous Navigation Link (and also you can merge the text of two links now). I'm not done yet, though. Removing/merging causes error in the JS console which reads:
I'm not really sure what this means or how to fix it. Also, the cursor is indeed moved to the previous block, but it's put at the beginning of the text, rather than the end. Finally, if a Navigation Link has children, the children are deleted when merging into the previous Navigation Link. Is there any way to transfer them to the other block? |
This seems to be an implementation detail of the annotations system, which I'm not sure I've ever really looked at, so I don't have more information unfortunately! Looking at other blocks like
Yeah, that's curious. I'll try to have a look into it at some point
I suppose it's a new behavior to have a block that can be deleted in this way that might have inner blocks. I think it's ok to try out like it is initially, but we could maybe flag it for design feedback, either on this PR or as a separate issue. |
So how do I add the identifier prop? I can't seem to find any documentation about it. |
ZebulanStanphill
force-pushed
the
fix/21743
branch
from
bb599bb to
1d94b1b
Compare
|
I had hoped that a rebase and adjustment after #22292 would fix some of the issues in this branch, but I'm still running into all of the same issues as before. |
ZebulanStanphill
force-pushed
the
fix/21743
branch
from
1d94b1b to
1f45415
Compare
ZebulanStanphill
removed
the
Needs Design Feedback
|
Well, it turns out that all I had to do to fix the JS console errors was literally just add Merging/removing is now working as expected. There is still one minor issue: removing a Navigation Link nested inside another Navigation Link doesn't focus the RichText of the parent like I would expect. Actually, it's not really clear where exactly the focus is. You have to tab or shift tab to get back to the standard tabbable elements. This also happens when backspace-removing a text block in a Group, so the issue isn't isolated to the Navigation block; because of that, I don't think it's a blocker for this issue. I think this PR is now ready for review. |
ZebulanStanphill
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
ZebulanStanphill
force-pushed
the
fix/21743
branch
2 times, most recently
from
9fcd405 to
54f8900
Compare
ZebulanStanphill
force-pushed
the
fix/21743
branch
2 times, most recently
from
f354ba9 to
bface7b
Compare
ZebulanStanphill
changed the title
ZebulanStanphill
added
the
[Feature] Writing Flow
ZebulanStanphill
force-pushed
the
fix/21743
branch
3 times, most recently
from
28de2ef to
132d1c2
Compare
ZebulanStanphill
changed the title
|
I updated this PR, using a similar approach as #22436. It's now working much better. I've updated the PR title and description to better describe all the behavior this PR adds. |
There was a problem hiding this comment.
Thanks for persisting on this @ZebulanStanphill! Looks like the PR now also fixes #18208, so I've updated the description.
If we can fix the naming I'd be very happy to approve.
| merge( a, { label: bLabel = '' } ) { | ||
| return { | ||
| ...a, | ||
| label: a.label + bLabel, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
I see the variable names are used in the other PR, but it's against the coding standards and it should probably have been caught during review of that:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#naming-conventions
Variable and function names should be full words, using camel case with a lowercase first letter.
I'm guessing here that the object is an attributes object.
I've seen left and right (e.g. leftAttributes, rightAttributes) used to distinguish before with similar operations, like when sorting, which I think makes sense here as there's a left block and a right block being merged.
There was a problem hiding this comment.
Thanks; I've revised the variable names.
ZebulanStanphill
force-pushed
the
fix/21743
branch
from
132d1c2 to
6e8ea2e
Compare
Description
Closes #21743.
Closes #18208.
This PR adds several RichText behavior enhancements to Navigation Link blocks. It introduces the following behaviors:
Notably, the ability to split an existing Navigation Link into two has been omitted, since this would cause a loss of all its children. I have no idea how to add this behavior and preserve children; I suspect it may not be possible to fix without improvements to the RichText APIs.
Checklist: