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.

Sign up for GitHub

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

Jump to bottom

Force Link UI to appear below a link if one is selected #6113

Merged
merged 1 commit into from
Apr 16, 2018
Merged

Force Link UI to appear below a link if one is selected #6113

merged 1 commit into from
Apr 16, 2018

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Apr 11, 2018 •
edited
Loading

Fixes #6028 (comment).

Before:

before

Now:

link

@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Apr 11, 2018
@gziolo gziolo requested a review from ellatrix April 11, 2018 05:33
karmatosed
karmatosed approved these changes Apr 11, 2018
View reviewed changes
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design wise this seems to have solved the dancing link issue. I'll let a code review happen also though.

@ellatrix
Copy link
Member

Hm, I'm not entirely sure how to test. I still see the link toolbar moving around.

link

@ellatrix
Copy link
Member

Nevermind, I didn't check the branch out properly.

@ellatrix
Copy link
Member

Still has some issues. Not sure why we are keeping the getRectangleFromRange at all. Isn't this only used for the link?

link

ellatrix
ellatrix requested changes Apr 11, 2018
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #6113 (comment). I think we can drop the calculation if none of the parents is a link (it's only used for links and there is no way this could be generalised). We can't just look at the element because there might be nested tags.

@noisysocks
Copy link
Member Author

Thanks @iseulde. I've fixed the issue with nested tags—great catch.

We can't drop the getRectangleFromRange calculation entirely, however, since the Link UI needs to be positioned underneath the selection when you are creating a new link, e.g:

link

ellatrix
ellatrix approved these changes Apr 13, 2018
View reviewed changes
Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go, but please fix the lining error first. :)

@noisysocks
Force Link UI to appear below a link if one is selected
c2d99d8 
Add custom logic that makes the FormatToolbar render the Link UI under
the link node if a link is focused. If something else is focused,
position the Link UI underneath the cursor or selection. This stops the
Link UI from dancing around when you click on a link.
@noisysocks noisysocks merged commit 6ae48b4 into master Apr 16, 2018
@noisysocks noisysocks deleted the fix/dancing-link-interface branch April 16, 2018 00:42
@noisysocks
Copy link
Member Author

Thanks @iseulde!

@noisysocks noisysocks added this to the 2.7 milestone Apr 19, 2018
@noisysocks noisysocks mentioned this pull request Oct 25, 2018
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellatrix ellatrix ellatrix approved these changes

@karmatosed karmatosed karmatosed approved these changes

Assignees
No one assigned
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@noisysocks @ellatrix @karmatosed