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

Ticket #1075: Accounting for allotted break time in estimated time to leave #1083

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yellowstrings
Copy link

Related issue

Closes #1075

Context / Background

  • Prior to this PR, the estimated time to leave did not account for the break interval set by the user. I've added that functionality in, while making sure not to extend the estimated time to leave after break, too.

What change is being introduced by this PR?

  • I approached this problem by first looking at the translation pages and searching "Based on the time you arrived today, you should leave by". From there, I found where the leave-by translation was used and what function was associated with calculating this time estimate.

  • If the break has not yet been taken, I'm converting the hours and minutes set for working hours and break intervals into number type, then adding them together to calculate the total amount of time, before then converting them back into a time string.

  • If the break has been taken, I am using the old code to only factor in working hours per day for the estimated time to leave.

  • I am using values.length to determine whether the break has been taken, so this requires values.length to stay an array. In addition, the break interval is factored into to the estimated leave time from the very start of the day. The only way that this will change is if a shorter or longer than expected break is taken by the user.

How will this be tested?

  • I ran all of the Jest tests. One of them, titled "emit RESIZE_MAIN_WINDOW", failed. This test has nothing to do with my code, but rather to do with the size of my app window when opened.

I tested my code manually by changing the break interval and working hours in my user settings. Below are the results:

Before any break is taken, based on an 8 hour work day

Screenshot 2024-10-16 at 12 30 51 PM

After a longer than expected break, based on an 8 hour work day

Screenshot 2024-10-16 at 12 31 06 PM

After a shorter than expected break, based on a 7 hour and 15 minute work day

Screenshot 2024-10-16 at 12 51 06 PM

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

Please make sure the changes in the package-lock.json are reverted.

Also, doesn't the original issue mention a toggle in the preferences?

Comment on lines +665 to +666
const [workingHours, workingMinutes] = hoursSet.split(":").map(Number);
const [breakHours, breakMinutes] = breakSet.split(":").map(Number);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we have functions on our time files to handle these?

Copy link
Author

@yellowstrings yellowstrings Nov 15, 2024

Choose a reason for hiding this comment

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

Good catch - I missed that part about the toggle! I wonder how necessary that is, though. I can't imagine a user deciding that they would rather keep an inaccurate end time to their day. What are your thoughts?

If possible, would you be able to point me to the files that handle these specific lines?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch - I missed that part about the toggle! I wonder how necessary that is, though. I can't imagine a user deciding that they would rather keep an inaccurate end time to their day. What are your thoughts?

I'm not sure. I don't quite remember the functionality of the enforced break, will have to run some tests to verify

If possible, would you be able to point me to the files that handle these specific lines?

I'd expect it to be at js/time-math.js

tupaschoal

This comment was marked as duplicate.

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.

Add estimated break time to time to leave
2 participants