-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Add a remove button to the link tooltip #256
Conversation
I think you might be able to reuse some of the saveLink code? In its case it changes the url whereas this removes it but both needs to find the anchor tag, figure out its range and call formatText on it. |
I extracted the I've tried a couple times to DRY this up, but saveLink and removeLink are really looking for different things. saveRange needs the anchor node in order to set the href, while removeLink just needs the range of the enclosing leaf. Ideally, I'd like to be able to use _findAnchor from within removeLink, but I'm not sure how to get a Quill range from a DOM node. Maybe we could add a doc.findRangeAt(node) function... |
Any more thoughts on this @jhchen? |
return unless range and !range.isCollapsed() | ||
if value | ||
return unless range | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove this empty line?
Can you also add 4 pixels of space between the "Change" and "Remove" text (perhaps margin-right on Change)? |
It looks like the over-zealous whitespace normalization from #149 is a problem here too. Wouldn't it be better to not call |
I think it's better to use css to create the space so |
OK. I can add a css rule as you suggest. I think it might be easier for contributors to have the markup act more normally (with standard whitespace semantics). What purpose does stripWhitespace serve here? |
Could be personal preference but I've never relied on whitespace in code to produce visual space. Many HTML compression tools get rid of them and explicit margin/padding has always been more reliable in my experience. |
Whitespace in between inline tags is significant. e.g. This results in output like "BoldItalic", instead of the expected "Bold Italic". |
I'm not saying its insignificant I'm saying I haven't found it to be reliable. |
Add a remove button to the link tooltip
Uses
@quill.editor.doc.findLeafAt(range)
to expand the selection to include the entire contents of the anchor (assuming that the leaf found is the anchor). This assumption makes me kind of nervous... but I couldn't come up with a better way to do this.Clicking the remove button in the tooltip always removes the link from the entire leaf.
Clicking the button in the WYSIWYG bar can remove the link from just the selected text (if the collection is not collapsed), otherwise it acts similarly to the tooltip remove button.