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

Fix selection of embedded objects. #359

Closed
wants to merge 1 commit into from

Conversation

willrowe
Copy link
Contributor

@willrowe willrowe commented May 7, 2015

This fixes an issue where the image toolbar button does not become active when an image is selected. You can reproduce this by going to the Quill homepage and selecting the image there, you will see that the image button stays inactive. This also prevents the image from being able to be deleted by clicking the image button while active.
I found that the issue is due to this line which was added to fix #351. The toolbar automatically expands a collapsed selection in order to display correctly when next to formatted text, so before this line of code was added to allow for double insertion of images from the toolbar, you could have your cursor next to an image and the toolbar button would be active (which I thought was unexpected behavior anyways).
This more permanently fixes #351 as well as any other types of embeds from now on, while still allowing the button to display as active if the image is indeed selected.

@willrowe
Copy link
Contributor Author

willrowe commented Jun 3, 2015

Any feedback on this?

@jhchen
Copy link
Member

jhchen commented Jun 5, 2015

After surveying other editors, it's not clear it is necessary or expected behavior for the toolbar button to light up when around an image. With formatting functionality, it is important to know if selected text already has the format since it is a toggle and thus changes the behavior of clicking the button. With insert functionality, the buttons should always just insert the embed.

Quill only has one embed at the moment which makes the distinction difficult but its current behavior does seem to be the common behavior amongst rich text editors, for example CKEditor and Gmail composer. Were you basing the PR behavior off of another editor or just an evolution of Quill behavior?

@willrowe
Copy link
Contributor Author

willrowe commented Jun 5, 2015

That's a good point as far as it being a way to insert and nothing else. If that's the direction that you want to go then the current behavior would be fine.
The reason I created this PR was to have consistent functionality for all toolbar formats, seeing as they display the state of the selected text. Unlike other editors, Quill has a more free-form toolbar that doesn't limit where the buttons are placed. In Gmail for example, the button to insert an image is on a separate toolbar from the text formatting.
Also, there is code to delete an image when the button is pushed and an image is already selected, so to me it makes more sense to display that an image is selected so you know if you click it that you'll essentially be toggling it off.
I was actually impressed to see this functionality as it is not normal in other editors and I think it would be a shame to remove it. If more functionality is added to the image tooltip in the future (setting an alt or title tag, choosing a different image, etc.) the button could be used to pull up the tooltip again to edit those values.
To sum it up, my thoughts behind this were to keep consistency between formats and allow for more robust handling of images.

@jhchen
Copy link
Member

jhchen commented Jun 5, 2015

It looks like even other open source rich text editors have an insert vs. format distinction and for now I think Quill's default behavior should remain similar. It would make more sense if Quill had a better image tooltip and/or other embeds that could be inserted which will eventually be the case. I'm hoping to go through all the modules including the toolbar and image tooltip after the custom formats work is done and make some much needed improvements.

@jhchen jhchen closed this Jun 5, 2015
@willrowe willrowe deleted the fix-embed-selection branch June 29, 2015 02:01
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.

Cannot embed two photos in a row due to photo button click issue
2 participants