Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/jpbulman/only-the-recipe/8HaxAJFGLcUVYAAyt4eTymntsVXS |
jpbulman
left a comment
jpbulman
left a comment
There was a problem hiding this comment.
Overall looks pretty good to me, thanks for the contribution 🎉 ! Just fixing the merge conflict and the annoying edge case should be enough for me
lib/domain/selectors.ts
Outdated
| ingredientsSelector: 'li[class="wprm-recipe-ingredient"]', | ||
| directionsSelector: 'div[class="wprm-recipe-instruction-text"]', | ||
| }, | ||
| 'seriouseats.com' : { |
There was a problem hiding this comment.
Would you mind alphabetizing this within the array? It helps avoid merge conflicts in prs 🙂
lib/domain/selectors.ts
Outdated
| }, | ||
| 'seriouseats.com' : { | ||
| titleSelector: 'h1.heading__title', | ||
| ingredientsSelector: '.section--ingredients .structured-ingredients__list-item', |
There was a problem hiding this comment.
This appears to break on some recipes because for some reason they use different classes for the list occasionally. I think it would probably work to just take the ingredients section, take whatever unordered list is in it, and then the list items.
|
Awesome, thanks so much for the fix 🔧 ! This looks great to me, the only thing that needs to be fixed is the alphabetical order on the domain list(s) so there aren't merge conflicts. After that, I'll put it through! |
2 participants
Issue #7