-
Notifications
You must be signed in to change notification settings - Fork 92
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
Choose duration for tomato #291
Choose duration for tomato #291
Conversation
See #77 |
Can you include screenshots for this feature? |
type="range" | ||
min="5" | ||
max="60" | ||
value="5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value should be 25.
app/models/tomato.rb
Outdated
@@ -11,6 +11,8 @@ class Tomato | |||
|
|||
index(created_at: 1) | |||
|
|||
field :duration, type: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Integer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've seen it after do the pull request, should be Integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a Tomato
model validation and a lot of counters that rely on the default duration.
Yes, there is the Workable module too, that I can't figure out how to adapt. |
It was counting in seconds and calculating in minutes
@matheussilvasantos cool, thanks uploading some screenshots and the updates. I will update the description to include all the things that I still think need to be done. |
max="60" | ||
value="5" | ||
oninput="duration_time_output.value = duration_time_input.value"> | ||
<output style="display: inline-block;" name="duration_time_output" id="duration_time_output">5</output> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more clear if we add a label, duration, and a unit of measurement, minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must_not_overlap
won't work without storing tomatoes on timer start.
There's one big blocker: if we don't store a new tomato as soon as a user starts the timer, the
A possible solution, that would make also resuming timers (#219) possible, would be to store a new tomato when the timer starts, and to update it when it ends. |
Include a label, "Duration", and a unit of measurement, "minutes".
Conflicts with must_not_overlap if counts in seconds just in view.
I don't know if I figured out what Storing a new tomato when the time starts is a big deal to implement here, I think this should be implemented with other pull request. |
Thanks so much @matheussilvasantos for your contribution, but I don't think I have the time right now to work towards the goal of making this PR mergeable in master. If @dalpo, or someone else, has some time to spent to think about a migration strategy for this PR let's keep this open, otherwise I will close it. I'm really sorry to "waste" the time you spent working on this, but I think this was a task too bit to be addressed without a clear migration strategy in place. If you'd still like to get involved in the project, while we find a solution to make this work, take a look at the other issues in the repo. |
Closing due to inactivity. |
Idea naively implemented to choose the duration of tomato
TODO
duration
paramApi::Presenter::Tomato
to includeduration
Tomato#must_not_overlap
validation method to use instance'sduration
Tomato::DURATION
as the default duration value