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

use single quote to fix deface override, see #2360 #2361

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

afdev82
Copy link
Contributor

@afdev82 afdev82 commented Nov 7, 2017

This change seems to fix the #2360 (and so the solidusio-contrib/solidus_editor#12)

@jhawthorn
Copy link
Contributor

jhawthorn commented Nov 8, 2017

I'd prefer this was fixed in deface. There's nothing wrong with our ERB here, deface just doesn't parse it correctly.

It seems like these regexes https://github.com/DefaceCommunity/deface/blob/master/lib/deface/parser.rb#L14-L16
are incorrect

They might need to be

/([\w-]+)(\s?=\s?)(")(([^"]*<%.*?%>)+[^"]*)/m,
/([\w-]+)(\s?=\s?)(')(([^']*<%.*?%>)+[^']*)'/m,
/([\w-]+)(\s?=\s?)()(<%.*?%>)(?:\s|>|\z)/m

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks. I am pro this change even without Deface being the source of failure. The mixed quotes in the html vs. Erb are confusing and will probably trip up syntax highlighting as well

@mamhoff
Copy link
Contributor

mamhoff commented Nov 8, 2017

Oh I had no idea how terrible Deface is. Still, pro this for the reasons @tvdeyen points out.

@jhawthorn jhawthorn merged commit c439e0a into solidusio:master Nov 8, 2017
@afdev82 afdev82 deleted the fix_solidus_editor branch November 10, 2017 08:09
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.

4 participants