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

Choose duration for tomato #291

Closed
wants to merge 11 commits into from
Closed

Choose duration for tomato #291

wants to merge 11 commits into from

Conversation

matheussilvasantos
Copy link

@matheussilvasantos matheussilvasantos commented May 13, 2017

Idea naively implemented to choose the duration of tomato

Home page
Tomato page

TODO

  • Update API documentation to include the new duration param
  • Update Api::Presenter::Tomato to include duration
  • Update Tomato#must_not_overlap validation method to use instance's duration
  • Use Tomato::DURATION as the default duration value
  • Update projects counters to take tomatoes duration into account
  • Update timer's duration output element to include a label, "Duration", and a unit of measurement, "minutes"

@potomak
Copy link
Member

potomak commented May 13, 2017

See #77

@potomak
Copy link
Member

potomak commented May 13, 2017

Can you include screenshots for this feature?

type="range"
min="5"
max="60"
value="5"
Copy link
Member

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.

@@ -11,6 +11,8 @@ class Tomato

index(created_at: 1)

field :duration, type: String
Copy link
Member

Choose a reason for hiding this comment

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

Why not Integer?

Copy link
Author

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

Copy link
Member

@potomak potomak left a 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.

@matheussilvasantos
Copy link
Author

Yes, there is the Workable module too, that I can't figure out how to adapt.

@potomak
Copy link
Member

potomak commented May 15, 2017

@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>
Copy link
Member

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.

Copy link
Member

@potomak potomak left a 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.

@potomak
Copy link
Member

potomak commented May 15, 2017

There's one big blocker: if we don't store a new tomato as soon as a user starts the timer, the must_not_overlap validation won't work in conjunction with a custom tomato duration.

must_not_overlap validation relies on the fact that all tomatoes last 25 minutes. This information makes it possible to validate tomatoes even if the current tomato has not been saves yet. With a custom tomato duration this requirement is missing.

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.

@matheussilvasantos
Copy link
Author

I don't know if I figured out what must_not_overlap does, but I think that it is working with the last changes.

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.

@potomak
Copy link
Member

potomak commented Oct 11, 2017

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.

@potomak
Copy link
Member

potomak commented Nov 27, 2017

Closing due to inactivity.

@potomak potomak closed this Nov 27, 2017
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