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

Make default cheerio options not escape characters #37

Closed
wants to merge 4 commits into from

Conversation

kball
Copy link
Contributor

@kball kball commented May 5, 2016

Fixes the escaping issues we've been seeing.

@westonganger
Copy link

+1

brandonbarringer pushed a commit to brandonbarringer/inky that referenced this pull request Jun 7, 2016
@Twissi
Copy link

Twissi commented Jun 7, 2016

👍 great stuff

@dhigginbotham
Copy link

💯 This issue has caused me quite a great amount of silly explanations for our current work around. 👍

@axelson
Copy link

axelson commented Jun 15, 2016

What is the status on this? This is a show-stopper for us. It looks like the Milestone is 2.1.1, but the current release of Inky is 1.3.5?

@axelson
Copy link

axelson commented Jun 15, 2016

Also I did some testing with this PR (actually with the fix-character-escaping branch) and it is better but it still doesn't support ERB syntax. e.g. <%= event.name %>, although perhaps that is out of scope of this PR.

@kball
Copy link
Contributor Author

kball commented Jun 17, 2016

This turned out to be incomplete because by making cheerio be more stringent, we then resulted in errors with stuff embedded within that was invalid xml (like ERB, or <> stuff). So to work around that, I implemented a tag that just forces some stuff to not be parsed. Going to resubmit this branch against develop. To do this I needed to slightly rework how inky interacts with cheerio (moving the parsing internal to Inky rather than external) so this will necessitate a change in the template as well.

@kball kball closed this Jun 17, 2016
@axelson
Copy link

axelson commented Jun 18, 2016

That makes sense. Thanks.

For reference new PR is #57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants