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.

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

Merged
merged 4 commits into from Jan 6, 2015

Conversation

thomsbg
Copy link
Contributor

@thomsbg thomsbg commented Nov 21, 2014

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.

@thomsbg thomsbg mentioned this pull request Nov 25, 2014
@jhchen
Copy link
Member

jhchen commented Nov 26, 2014

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.

@thomsbg
Copy link
Contributor Author

thomsbg commented Nov 26, 2014

I extracted the _expandRange function and synchronized the toolbar correctly.

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...

@thomsbg
Copy link
Contributor Author

thomsbg commented Dec 4, 2014

Any more thoughts on this @jhchen?

return unless range and !range.isCollapsed()
if value
return unless range

Copy link
Member

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?

@jhchen
Copy link
Member

jhchen commented Dec 4, 2014

Can you also add 4 pixels of space between the "Change" and "Remove" text (perhaps margin-right on Change)?

@thomsbg
Copy link
Contributor Author

thomsbg commented Dec 4, 2014

It looks like the over-zealous whitespace normalization from #149 is a problem here too. Wouldn't it be better to not call stripWhitespace() on tooltip templates?

@jhchen
Copy link
Member

jhchen commented Dec 8, 2014

I think it's better to use css to create the space so stripWhitespace() shouldn't be a problem here?

@thomsbg
Copy link
Contributor Author

thomsbg commented Dec 8, 2014

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?

@jhchen
Copy link
Member

jhchen commented Dec 8, 2014

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.

@thomsbg
Copy link
Contributor Author

thomsbg commented Dec 8, 2014

Whitespace in between inline tags is significant.

e.g. <b>Bold</b> <i>Italic</i> is being transformed to <b>Bold</b><i>Italic</i>.

This results in output like "BoldItalic", instead of the expected "Bold Italic".

@jhchen
Copy link
Member

jhchen commented Dec 9, 2014

I'm not saying its insignificant I'm saying I haven't found it to be reliable.

jhchen added a commit that referenced this pull request Jan 6, 2015
Add a remove button to the link tooltip
@jhchen jhchen merged commit 8deeef7 into quilljs:develop Jan 6, 2015
@thomsbg thomsbg deleted the remove-link branch January 27, 2015 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants