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:311-add preserveWhitespace prop #407

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

royshouvik
Copy link
Contributor

Adds preserveWhitespace prop as discussed in #311

@royshouvik
Copy link
Contributor Author

@alexkrolick does this PR look reasonable? It would be nice to have it merged, we are facing a annoying bug because of this.

@alexkrolick
Copy link
Collaborator

Does it work after restoring the content? I thought Quill would ultimately collapse whitespace by itself.

@royshouvik
Copy link
Contributor Author

Yes, it does work after restoring the content. Quill has a check for PRE node here, and doesn't collapse whitespace if the parent node is a PRE tag. Should I add a minimal working example?

@alexkrolick
Copy link
Collaborator

Yes please

@royshouvik
Copy link
Contributor Author

@alexkrolick here is an working example of the preserveWhitespace prop. Please let me know if you have any questions.

@ramospedro
Copy link

ramospedro commented Oct 15, 2018

Hey! It's very nice to have someone working on this issue. I had some trouble with whitespaces as well. But I have to bring my point of view on this issue.

We should notice that using a pre instead of a div is something that might have unexpected consequences, so it's weird to have a prop "preserve whitespaces" implicitly making a change that should be explicit. I think it needs more discussion.
It'd be great if we can have a simple prop like that to control whitespaces, but the presented solution looks to me like we're wrapping up a workaround. As mentioned here, this only works because of specific styling details.
Also, if a developer wants to render a pre instead of a div (for any means), one already has a way of doing it (by passing a children). For instance, what happens if the developer passes a child to render instead of the defailt div and also sets this prop to true?

@royshouvik
Copy link
Contributor Author

royshouvik commented Oct 16, 2018

@ramospedro Thanks for your inputs.

this only works because of specific styling details

I would disagree. If you check the this line in Quill, it specifically checks for a PRE node and not the CSS whitespace property.

I agree that passing a pre node as a child is a possible solution. However, because of the focus loss bug in React 16, its not usable at all.

Do you have any suggestion around how we can fix this in a better way?

@ramospedro
Copy link

@ramospedro Thanks for your inputs.

this only works because of specific styling details

I would disagree. If you check the this line in Quill, it specifically checks for a PRE node and not the CSS whitespace property.

I agree that passing a pre node as a child is a possible solution. However, because of the focus loss bug in React 16, its not usable at all.

Do you have any suggestion around how we can fix this in a better way?

Sorry I didn't notice this before. So, according to this, Quill.js explicitly removes whitespaces only when it's not a pre tag. That's it, then. I think I have nothing else to say :)

@alexkrolick alexkrolick merged commit 916a18e into zenoamaro:master Oct 16, 2018
@alexkrolick
Copy link
Collaborator

Thanks, let's go with this then.

@royshouvik royshouvik deleted the 311-preserver-whitespace branch January 30, 2019 18:41
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

3 participants