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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fallback for unstructured Jamie Oliver recipes #1

Merged
merged 1 commit into from Oct 25, 2018

Conversation

harmenjanssen
Copy link
Contributor

Note: misses proper white-space handling.
How would you like to deal with that? Right now I strip all HTML tags from it, ending up with illegible output.
Maybe it would be nice to create some kind of Response object, where isRawHtml is a property, so you can differentiate the way you render it in the app.
That would mean touching every service in the API though, as well as the output rendering.

Well, anyways, this at least ensures the body isn't empty. 馃檪

Copy link
Owner

@mattijsbliek mattijsbliek left a comment

Choose a reason for hiding this comment

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

The HTML of these recipes is pretty gnarly. Keeping line breaks intact seems like our best option for now.

I experimented with trying to extract the individual steps by regexing [0-9]\. patterns, but it鈥檚 so specific to this one recipe I don鈥檛 think it鈥檚 worth the effort.

The formatting did got me thinking about allowing sanitized , , and tags in the front-end, maybe even . This would probably help with formatting.

services/jamie-oliver/properties/recipeInstructions.js Outdated Show resolved Hide resolved
services/jamie-oliver/properties/recipeInstructions.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@harmenjanssen
Copy link
Contributor Author

I experimented with trying to extract the individual steps by regexing [0-9]. patterns, but it鈥檚 so specific to this one recipe I don鈥檛 think it鈥檚 worth the effort.

Oh I definitely wouldn't do that, it's way too specific.

Your sanitizeHTML function works wonders on that particular recipe! Looks fine now.
Let me know if you're approving these changes, so I can rebase all those separate commits into a sensible one. 馃憤

@mattijsbliek
Copy link
Owner

LGTM 馃帀 If you can rebase I鈥檒l merge and deploy

Note: misses proper white-space handling.
@harmenjanssen
Copy link
Contributor Author

Yeah! Go for it! 馃檪

@mattijsbliek mattijsbliek merged commit 52d9e81 into mattijsbliek:master Oct 25, 2018
@mattijsbliek
Copy link
Owner

Deployed, thanks for contributing 鉂わ笍

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.

None yet

2 participants