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

@kanaabe => Strip unicode spaces #1560

Merged
merged 4 commits into from
Jan 24, 2018
Merged

@kanaabe => Strip unicode spaces #1560

merged 4 commits into from
Jan 24, 2018

Conversation

eessex
Copy link
Contributor

@eessex eessex commented Jan 24, 2018

Json stringify seems to have issues with unicode spaces 2028 and 2029.
(https://stackoverflow.com/questions/16686687/json-stringify-and-u2028-u2029-check)

This adds a check specifically for those characters and removes them on paste.

Related to #1403

@ghost ghost assigned eessex Jan 24, 2018
@ghost ghost added the In progress label Jan 24, 2018
@kanaabe
Copy link
Contributor

kanaabe commented Jan 24, 2018

According to the link it doesn't seem like the behavior should be to strip it but instead escape it properly? I'm not too familiar with this, but if the character is presented somewhere like Google Docs correctly, it could cause unexpected changes to the text if we strip it completely.

Also it looks like the tests are checking if the function is called but doesn't actually test the code change.

@eessex
Copy link
Contributor Author

eessex commented Jan 24, 2018

Rather than escaping linebreaks (which could potentially be outside our spacing rules), added a new function that replaces them with an empty paragraph. This was the spacing should be preserved, but also stay within our ruleset. This is called on mount as well as on paste, so hopefully we should be good.

@eessex eessex force-pushed the unicode branch 2 times, most recently from 80c9fd3 to 4203356 Compare January 24, 2018 20:51
Copy link
Contributor

@kanaabe kanaabe left a comment

Choose a reason for hiding this comment

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

Haven't seen a for loop like that in a while 😄 . I like the replacement though -- I couldn't come up with any edge cases with the loop + nested tags so this LGTM! 👍

@kanaabe
Copy link
Contributor

kanaabe commented Jan 24, 2018

I'm sure you've tested that this works locally, but I was thinking there's probably a few articles still out there with this character and it'd be a good opportunity to search for them in Robomongo to clean up entirely. Happy to do this part with you sometime!

@eessex eessex merged commit b5faffc into artsy:master Jan 24, 2018
@ghost ghost removed the In progress label Jan 24, 2018
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.

2 participants