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

Adds Clever Dripper recipe #179

Conversation

MarcBernstein
Copy link
Contributor

Description

Adds Clever Dripper recipe from https://youtu.be/RpOdennxP24

Type of change

  • New feature

Checklist:

  • I've added new item into Changelog under [Unreleased]
  • I've tested the change on device

@rozPierog
Copy link
Owner

Thanks @MarcBernstein
Did you get written permission from James and/or his team to use this recipe in the app?
That's the process I followed for all the recipes in the app, and I want to be fair to him and his hard work.
I would appreciate if you could attach an "ok" from them to this PR so I know that it's okay 😅

@MarcBernstein
Copy link
Contributor Author

Thanks @MarcBernstein Did you get written permission from James and/or his team to use this recipe in the app? That's the process I followed for all the recipes in the app, and I want to be fair to him and his hard work. I would appreciate if you could attach an "ok" from them to this PR so I know that it's okay 😅

Sounds good @rozPierog! Would you mind sharing the contact info (or method you used to work with them before) so that I can also reach out? I want to make sure they're good with it too. If not that's fine too, I should have reached out to them before creating this. I had mistakenly thought there was more of a blanket permission here in the app as long as proper attribution was given.

@rozPierog
Copy link
Owner

No worries @MarcBernstein !

I used contact form on his site https://www.jameshoffmann.co.uk/contact-me and it was back in 2021-01-18, so long time ago, I just want to make sure that nothing has had changed.
I've picked a "Brand contact" option as it best reflected my query at the time but maybe some of the new options would be better now.
Feel free to remind them that I contacted them back then, and to CC me to the conversation if needed (<my_github_username>@gmail.com)

@MarcBernstein
Copy link
Contributor Author

Great, that's sent off and we'll wait to see how they respond. Appreciate you walking me through that process.

@rozPierog
Copy link
Owner

Ok, we have a green light from James, I'll move forward with code review and I hope we will merge it and release this week ☕️

@@ -26,6 +26,7 @@ enum class RecipeIcon(@DrawableRes val icon: Int, @StringRes val nameResId: Int)
Siphon(R.drawable.recipe_icon_siphon, R.string.name_siphon),
Bripe(R.drawable.recipe_icon_bripe, R.string.name_bripe),
Cezve(R.drawable.recipe_icon_cezve, R.string.name_cezve),
CleverDripper(R.drawable.recipe_icon_drip, R.string.prepopulate_clever_dripper_name),
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't add new icons if they use the same drawable. In this case drip icon will show up twice in the icon picker

Maybe in the future I could hire Hubert again for new icons then I'll add Clever Dripper here.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it would be best to change V60 icon into a generic drip icon, as there are a lot of similar looking drips on the market and it makes no sense to create an icon for every single one of them. WDYT?

@@ -149,6 +149,12 @@ fun DefaultPreview() {
description = stringResource(R.string.prepopulate_aero_description),
recipeIcon = RecipeIcon.AeroPress,
),
Recipe(
Copy link
Owner

Choose a reason for hiding this comment

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

That was unnecessary but I appreciate the effort of adding it to the preview 🤎

Copy link
Owner

@rozPierog rozPierog left a comment

Choose a reason for hiding this comment

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

I'm gonna go ahead and merge it and fix my icon issue on my own. Once again thanks @MarcBernstein

@rozPierog rozPierog merged commit 618bccf into rozPierog:main Jul 1, 2023
1 of 3 checks passed
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