Make Link UI inactive if selection extends beyond format bounds or encounters new link #35946
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
getdave
added
[Feature] Link Editing
|
Size Change: +138 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
getdave
requested review from
ajitbohra,
cameronvoell,
ellatrix,
guarani,
nerrad and
ntwb
as code owners
7 tasks
getdave
changed the title
getdave
added
[Package] Block editor
getdave
commented
Oct 26, 2021
| } ); | ||
|
|
||
| if ( | ||
| ! linkFormatAtStart || |
There was a problem hiding this comment.
If no format here then we've exceeded the leftmost side of the link boundary.
getdave
commented
Oct 26, 2021
|
|
||
| if ( | ||
| ! linkFormatAtStart || | ||
| ! linkFormatAtEnd || |
There was a problem hiding this comment.
If no format here then we've exceeded the rightmost side of the link boundary.
getdave
commented
Oct 26, 2021
| if ( | ||
| ! linkFormatAtStart || | ||
| ! linkFormatAtEnd || | ||
| linkFormatAtStart !== linkFormatAtEnd |
There was a problem hiding this comment.
If they do not display referential equality then we're dealing with two separate links.
There was a problem hiding this comment.
Does this mean that if the links are the same the UI shows?
There was a problem hiding this comment.
Screen.Capture.on.2021-10-26.at.14-09-46.mov
Nope. Not in my testing. This is because the two link format objects are not referentially equal (i.e. they are not the same format object).
Note in the video above the two items I'm console.loging are linkFormatAtStart and linkFormatAtEnd
getdave
commented
Oct 26, 2021
| // the bounds of the link format. | ||
| // Also if the format objects don't match then we're dealing with two separate | ||
| // links so we should not allow the link to be modified over the top. | ||
| if ( name === 'core/link' && ! isCollapsed( value ) ) { |
There was a problem hiding this comment.
No need to handle collapsed selections because for these start and end will be identical and thus there is no "selection".
There was a problem hiding this comment.
Could we remove the hard coding of a format name here?
There was a problem hiding this comment.
Sure. Are you suggesting to replace with a reference instead? Or were you suggesting something else?
There was a problem hiding this comment.
Why has this been merged while it's hardcoding link logic in the rich text component?
There was a problem hiding this comment.
Apologies. That must have been an oversight despite your comment here. Both Andrei and I must have missed it in the Github UI even though I responded 🤦
My question still stands though. How can we work around this?
There was a problem hiding this comment.
Not sure I understand the question 😅
In any case, I've made a PR here: #48789
There was a problem hiding this comment.
Not sure I understand the question 😅
Neither do I nearly a year down the line 😄 I think it was "do you have any thoughts on how we could proceed"...and I'm not surprised by the fact that you did (do!).
Thank you for following up here.
javierarce
approved these changes
Oct 26, 2021
draganescu
reviewed
Oct 26, 2021
draganescu
approved these changes
Oct 26, 2021
Co-authored-by: andrei draganescu <me@andreidraganescu.info>
|
Waiting on #35961 so we can skip the test. |
getdave
deleted the
fix/disable-link-format-edit-if-beyond-format-bounds
branch
andrewserong
added
the
[Type] Bug
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
This PR seeks to solve the problem highlighted by @ellatrix in #33849 (comment) (and which is currently blocking the merge of that important PR).
Currently if you make a link and then make a selection that begins inside the link and extends either:
...then the Link UI will remain active even though you're outside the original format you selected.
This allows you to get into undesirable states such as:
This PR resolves this. The UX is now that the Link UI will disappear (become inactive) if:
Originally part of #35882. Now broken out into a seperate PR.
How has this been tested?
Manual testing
Bottom line: if the selection goes:
...then the Link UI should disappear.
Screenshots
Before
Screen.Capture.on.2021-10-26.at.09-41-25.mov
After
Screen.Capture.on.2021-10-26.at.09-44-26.mov
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).