Skip to content

Conversation

@mihaiconstantin
Copy link
Contributor

Hi Chris,

As I mentioned in my email, I'm a big fan of your application.

I added a small counter to keep track of how many Pomodoro sessions are completed. Not sure if this is something you or other users might find useful, but I find it helpful to have a rough idea of how I did that day. I thought of sharing (see screenshot below).

total_sessions

Best,
Mihai

@mihaiconstantin
Copy link
Contributor Author

I realized this introduces a bug. Closing #84 and will send another PR with a better implementation.

@Splode
Copy link
Owner

Splode commented May 8, 2020

Thanks for the contribution, @mihaiconstantin.

Out of curiosity, what was the bug you encountered? I didn't see anything in the code that stood out.

@mihaiconstantin mihaiconstantin deleted the feature-total-sessions branch May 16, 2020 12:18
@mihaiconstantin
Copy link
Contributor Author

mihaiconstantin commented May 16, 2020

@Splode Thanks for asking!

I was updating the total rounds counter based on the INCREMENT_ROUND mutation. However, that mutation is only triggered 3 times per work session1 because the fourth time a reset is performed instead. So, every work session the total rounds counter would lag cumulatively. I think this is made clear by the console.log statements you added when you emit the events in Timer-controller.vue within method checkRound() (see screenshot below).

Looking at the code more carefully, I think a logical approach would be to:

  • initialize the total rounds counter at 0 and only display it if it's higher than 0
  • create a standalone mutation and action for the total rounds counter
  • dispatch this action every time before a short or long break, i.e., when this condition holds this.currentRound === 'work'

I will try to send another PR.

image

1By work session I mean 4 Pomodoros in a row.

Splode added a commit that referenced this pull request May 16, 2020
Added counter for total work sessions (follow-up #84)
@Splode
Copy link
Owner

Splode commented May 16, 2020

Good catch and thank you for the explanation!

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