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: Froala editor: useClassess to false #5174

Merged
merged 4 commits into from
Oct 12, 2018

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Oct 19, 2017

Q A
Bug fix? y
New feature?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs)
BC breaks?
Deprecations?

Description:

Just set useClassess option to false.
For this option to work correctly, it is necessary to load the CSS files from the same domain the editor is running on.
https://www.froala.com/wysiwyg-editor/docs/options#useClasses

Another option is uncommented these lines


I didn't know why it was commented. Should somebody from core team should know

Steps to reproduce the bug:

  1. Create form and create description area
  2. Paste image and try set align to right
  3. Nothing happend

Steps to test this PR:

  1. Apply PR and run app/console m:a:g
  2. Create form and create description area
  3. Paste image and try set align to right
  4. Should work properly

List deprecations along with the new alternative:

List backwards compatibility breaks:

@kuzmany kuzmany changed the title Froala editor: useClassess to false Fix: Froala editor: useClassess to false Nov 1, 2017
@alanhartless
Copy link
Contributor

The froala CSS files are not loaded into the production versions of the landing page/emails and so they can't be used. This was done on purpose to prevent froala from interfering with the them with it's own CSS.

@GaberNeighbor
Copy link
Contributor

This is a clean way to prevent froala from stripping inline css and applying classes a) are from a stylesheet we don't allow froala to generate b) are useless in email anyway

@escopecz
Copy link
Sponsor Member

This PR still hacks the Froala library. This option does not work?

@GaberNeighbor
Copy link
Contributor

@escopecz Is this a new addition? If this is currently implemented then no, it doesn't work.

@kuzmany
Copy link
Member Author

kuzmany commented Jan 25, 2018

@GaberNeighbor please try update. Don't forget call php app/console mautic:assets:generate

@escopecz escopecz added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Jan 25, 2018
@escopecz
Copy link
Sponsor Member

Woohoo, this looks much better!

@davevurby
Copy link
Contributor

Works properly. Problem from #5620 is working as well.

@escopecz
Copy link
Sponsor Member

escopecz commented Jan 25, 2018

So this solution works for the use case @kuzmany outlined. It does not solve @GaberNeighbor's use case. It would if we'd put useClasses: false, into 4.builder.js on line 1330.

However, all of this was already there once, but it was removed because it did not work for another use case. I'm not quite sure what to do now.

@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
@dbhurley dbhurley removed this from the 2.13.0 milestone Mar 27, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Jul 26, 2018

@escopecz But this option was removed before Code Mode. I
n that time Mautic allow edit all templates and then Froala editor destroy layout.
Mautic 2.3 allow edit just part of template, then this option shouldn't destroy layout.

@npracht npracht added this to the 2.14.2 milestone Sep 5, 2018
@escopecz escopecz added this to To Do in Testing 2.14.2 Oct 4, 2018
Copy link
Contributor

@Woeler Woeler left a comment

Choose a reason for hiding this comment

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

Solved the issue for me.

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Oct 12, 2018
@escopecz escopecz moved this from To Do to Tested Once in Testing 2.14.2 Oct 12, 2018
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Alright then 👍

@escopecz escopecz merged commit db04a77 into mautic:staging Oct 12, 2018
Testing 2.14.2 automation moved this from Tested Once to Merged Oct 12, 2018
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label Oct 12, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Dec 10, 2018

I noticed this PR generate a lot of inline styles.

  • With useClasses:false generate inline style from CSS include to editor event (just select H1 title in editor)

image

  • With useClasses:true generate class style to elements

This is real issue. I didn't find any option to disable it. But we should focus on it and include solution to 2.15 :(

@Woeler @escopecz

Test with forms and description area field. I tested with enable/disable useClasses

@escopecz
Copy link
Sponsor Member

@kuzmany what's the problem? Is it just longer HTML?

@kuzmany
Copy link
Member Author

kuzmany commented Dec 12, 2018

@escopecz Yes, it's bad. Style form by theme has to be with !important suffix. Inline style has priority, then we have to rewrite with important flag (color, font-size). And that not cool.
Maybe we should remove linked css file from editor, but I didn't have time to test it.

@escopecz
Copy link
Sponsor Member

So the emails/pages now looks differently? Are you able to send before and after screenshot? I mean it should look the same as in the builder, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs
Projects
No open projects
Testing 2.14.2
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants