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

1860 Remove style from pasted text for shopfront messages #4816

Merged
merged 4 commits into from
Mar 18, 2020
Merged

1860 Remove style from pasted text for shopfront messages #4816

merged 4 commits into from
Mar 18, 2020

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Feb 21, 2020

What? Why?

Closes #1860

When you edit the shopfront message and paste text or HTML into the editor, it preserves the formatting. It also preserved colours including backgrounds. That could lead to ugly messages. We now keep the basic formatting but remove all additional styles like colours and fonts.

Before:
Screenshot from 2020-02-21 14-16-46
After:
Screenshot from 2020-02-21 14-17-03

I also upgraded the editor library.

I did not change existing messages. Shopfront messages that already have weird formatting will keep their formatting. The solution is to go to edit the enterprise, select the whole text and click the clear formatting button, then save.

What should we test?

  • Go to edit an enterprise and edit the shopfront message and the shop closed message.
  • Test basic functionality of the editor to make sure that the update didn't break anything.
  • Test with different browsers (I'm not sure what our requirements are).
  • Paste plain text and verify that nothing is lost.
  • Paste colourful text, including background colours and make sure that the colours disappear.
  • Check that pasted bold text and links are preserved.
  • Save your result and and verify the display in the shopfront.

Check that other text fields using the same editor code work as before:

  • Enterprise about text
  • Enterprise group about text
  • New product form
  • Edit product form

Release notes

Changed: When you edit the shopfront message and paste text, it preserves the formatting but strips additional styles like colours and fonts.

Changelog Category: Changed

@mkllnk mkllnk self-assigned this Feb 21, 2020
@daniellemoorhead
Copy link
Contributor

Good job :)

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

"I also upgraded the editor library."
ideally we should be doing these upgrades in a dependencies list.
It looks like you have upgraded textAngular, textAngular-rangy and textAngular-sanitize. From what versions did you upgrade from/to exactly? and how did you find the code for the new versions?

This upgrade means we shoul verify all places where textAngular is used, right?

I'm not sure which version we had previously. This update should come
with lots of minor improvements even though we didn't have anyone
complain.
Our TextAngular module supplies that code.
@mkllnk
Copy link
Member Author

mkllnk commented Feb 24, 2020

@luisramos0

ideally we should be doing these upgrades in a dependencies list.

Yes. I didn't find a gem for it. We could start using another tool like bower but I'm not familiar with it and it's out of scope of this PR.

It looks like you have upgraded textAngular, textAngular-rangy and textAngular-sanitize. From what versions did you upgrade from/to exactly?

I have no idea from which version I upgraded. That was one of my reasons to upgrade. The commit message now contains the new version number: v1.5.16

and how did you find the code for the new versions?

The comment contains a link to the Github repository. I went to releases, downloaded the zip file and copied the minified files over.

This upgrade means we shoul verify all places where textAngular is used, right?

Yes, I'll add that to the test list.

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

ok, thanks Maikel for the answers. I didnt see the commit message and now I see that the link in the comment is not just for sanitize, is to download textAngular 👍

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

ideally we should be doing these upgrades in a dependencies list.

totally agree with @luisramos0 . The way to manage dependencies in Rails is through gems so I'd that in this case as well? I quick googling lead me to https://rubygems.org/gems/text-angular-rails/versions/1.5.16

@luisramos0
Copy link
Contributor

luisramos0 commented Feb 28, 2020

gems for javascript are not always up to date and the list is naturally not so complete as if we use bower or other js package manager.
I'd agree this change is out of scope for this PR but I think you are saying we just need to remove the texangular js code and add this to the gemfile:
gem 'text-angular-rails', '~> 1.5', '>= 1.5.16'

Copy link
Member

@kristinalim kristinalim left a comment

Choose a reason for hiding this comment

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

Nice work, @mkllnk! 🙂 I agree about not using gems for purely JS libraries, and that package management is outside the scope of this PR.

Just noting that Bower is deprecated, so maybe yarn would be good.

@luisramos0
Copy link
Contributor

@sauloperez are you ok with this? can we move to test ready?

@sauloperez
Copy link
Contributor

IMO it'd be much better if we used a package manager (preferably one that takes the asset pipeline into account) but we're not making our situation worse than it was and we are fixing a bug so 👍

@daniellemoorhead daniellemoorhead added the pr-staged-au staging.openfoodnetwork.org.au label Mar 18, 2020
@daniellemoorhead
Copy link
Contributor

TESTING NOTES

I pasted some green highlighted copy to a shop on AU staging (Carrot Farm) and the highlight did not display in the text field.

What I did notice, however, was that it pasted in bold even though the font in the Google Doc was not bold, it was normal.

So I tested this on another staging server, and it did the same thing. Thus, it's an existing bug and not a problem for this particular change.

@mkllnk this change is therefore good to go 🎉

@daniellemoorhead daniellemoorhead removed the pr-staged-au staging.openfoodnetwork.org.au label Mar 18, 2020
@mkllnk mkllnk merged commit b63c47c into openfoodfoundation:master Mar 18, 2020
@mkllnk mkllnk deleted the 1860-copy-paste branch March 18, 2020 05:23
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.

[mobile ux] Copy-paste in shopfront message display strange white strips
6 participants