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

Minor improvements #279

Merged
merged 3 commits into from
Dec 26, 2020
Merged

Minor improvements #279

merged 3 commits into from
Dec 26, 2020

Conversation

aarondoet
Copy link
Contributor

  • Add min (1) and max (999) to the servings spinner on the recipe view
  • Made the text next to the checkbox the label instead of the title for the All Keywords check on website import (fixes Unintuitive keywords label in website import #278)
  • Add the show class to the advanced search element if you searched with the advanced search

@aarondoet
Copy link
Contributor Author

Pretty sure there has to be an easier way to only include some commits in a pull request. After my first pull request I created a new branch to be able to make a second PR because they are unrelated and PRs are based on the branch. I then made these changes, pushed them and then in the overview when creating a new PR I noticed that the commits from my other PR are also included. Since the new commits did not change anything that was changed in my old commits I think it should be possible to just remove them but I really don't know. I then created a new branch that I based on the last commit that is not from me, remade and recommited the changes and then create a PR but I'm sure there has to be a better way. If you know anything please let me know. (Besides not forgetting about that when creating the new branch in the first place)

@vabene1111
Copy link
Collaborator

haha i had the same Problem with PRs in the past many time .. in the end i always went for the option you described and i am not really sure if there is a better way. This makes it especially complicated if a second PR is using something from a first one.

Regarding the changes:
Sounds really good, the only thing i would change is allow the scaling calculator to also go to 0.XX since sometimes it might be desired to cook half of a recipe (this is probably only relevant as long as it is the way it currently is where the scaling factor is not based on servings but on a fixed "recipe" amount, adding servings was discussed at length in the other open PR but i haven't gotten around to implementing it).

i hope that i will find some time over the christmas holiday to merge your PRs but i cant guarantee it

@aarondoet
Copy link
Contributor Author

Yeah you are right, I did not think about that. It should also be possible when the servings attribute is implemented, e.g. for something like cake or tarte most often 1 serving is used to indicate that you will get one of them with the ingredients and not how many serving that actually are. But maybe you only want to bake a small version of the cake.

Is there anything that allows you to specify custom step sizes? I'd like to make it that if the value is 1 and you go down it will decrease by 0.1 but it would increase by 1. Just changing the step size based on the value would not work, it also depends on the direction of the change. I tried overwriting the element's stepUp and stepDown function but apparently they are not used when clicking the arrows or using the arrow up/down keys. I don't want to create a custom component for this and I don't want to use a value other than 1 for increasing if the value already is bigger than 1.

@aarondoet
Copy link
Contributor Author

I'll just leave it as is for now because the previous version did not allow non-integer values either. You can input them manually and it will scale correctly but it will tell you that it is an invalid value. The same also applies to this input with min and max (at least for me on firefox, who knows if other browser decide to behave differently).

@vabene1111
Copy link
Collaborator

i would say it is perfectly reasonable to manually type in the value if you dont want a full integer 👍

@aarondoet
Copy link
Contributor Author

That's fair. Imo it shouldn't say that the value is invalid tho. But I can't find anything that would allow this. If you set the step property then only min + whole multiples of step and max are valid values. But apparently you can't set something like a step used for the validator and a step used by the browser to decide how much to increase/decrease so that steps inbetween are allowed too.

@vabene1111 vabene1111 merged commit bb48655 into TandoorRecipes:develop Dec 26, 2020
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.

Unintuitive keywords label in website import
2 participants