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

Solves #1830 (Split times over 60 min into hours and minutes) #1986

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

CameronJGrant
Copy link
Contributor

Adds an option to have time split into minutes and hours. It seems most languages understand "h", "min" notation, so I've stuck with that.

image
image
image
image

@vabene1111
Copy link
Collaborator

thank you, i really like this change and also the clean and easy to understand implementation. One thing up for debate: do we really need a setting for this ? i think we can just implement this as the default behavior, back when i migrated the features from django to vue I was simply to lazy to implement conversion, i dont see why any one would change this setting and it reduces complexity if we dont add it right ?

feedback appreciated

@CameronJGrant
Copy link
Contributor Author

I agree with you.

My reasoning for implementing it as a setting is that I'm reluctant to force change a feature of the UI that has been around this long. That is, maybe someone, somewhere in the world, actually likes their display being in minutes only. (This comes from personal experience of being forced into UI changes that a dev thought was "better". :P )

I'm sure I'm over thinking it for something this small

That said, I'd certainly be open to removing the option and keeping it as hour/minutes. Or perhaps, defaulting to hours/minutes display in models.py and hiding the setting somewhere.

I'll leave it up to you. Let me know and I'll change it.

@vabene1111
Copy link
Collaborator

Perfectly valid reasoning and I am not sure if it's just the frustration I had recently with the overall complexity or if it's actually the better decision but it would be awesome if you could remove the setting code.

We will have it in the pr forever so if there is a huge demand we can re add it any time.

Thanks a lot for the pr

@CameronJGrant
Copy link
Contributor Author

Sounds good! All set.

@vabene1111 vabene1111 merged commit 39070d3 into TandoorRecipes:develop Sep 9, 2022
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.

2 participants