-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add break timer #163
Add break timer #163
Conversation
…ak timer while the main timer is paused
…hout the settings grid
By the way, if it helps to reduce the effort on your end, I'm happy to have a call and discuss the changes :) |
I'm really on the fence with this feature. The code looks good, so I'm not concerned with that, but I'd want feedback on how mobs currently manage their breaks. For reference, I have at least 10 teams at work that manage breaks on their own while still using mobtime, so I'm curious what the value add is. |
Also, sorry it took me a while to get back to you on this! |
I'm currently running two mobs with this version of the timer (hosted at https://mobtime.herokuapp.com/ ). I know that others asked me for that version as well, but I didn't yet get feedback from them whether they actually use it.
|
Our mob would really appreciate this feature as well. Our concerns are the same as @dbartholomae 's; managing two timers is clunky (especially when the break timer is not shared), and being forced to take breaks is always nice. |
public/actions.break.test.js
Outdated
const initialState = { | ||
breakTimerStartedAt: new Date(), | ||
settings: { | ||
breaksEnabled: false, |
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.
Shouldn't this be true
?
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.
Good point, fixed
eb0d05c
to
bcae48a
Compare
So a lot has changed in the code-base since this PR. |
I've written a first implementation based on this proposal I added in #119. It would be helpful for me to have on Monday, but if you need more time, I will be able to live without it a bit longer ;)
I had to make some additional smaller clean ups around the the Timer subscription and usage of currentTime from state to be able to use the Timer subscription for multiple timers.