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

Enabling more formats to input hours and minutes in preferences #1020

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

Conversation

TaranDev
Copy link

Related issue

Closes #914

Context / Background

  • Previously the input of the "Hours per day" and "Break time interval" preferences sections were restricted to an XX:XX format, this PR introduces more formats that can be used.

What change is being introduced by this PR?

  • You can now use X, XX, XX.X, X.XX, X:XX, and XX.XX in addition to the default XX:XX format when entering hours and minutes into the "Hours per day" and "Break time interval" sections in preferences.
  • If the entered value matches one of these formats, it will be converted to match XX:XX before being saved as a preference.
  • This involved changing the pattern regex for those fields in preferences.html, and performing some case checks and string manipulation in preferences.js.

Before change:

Screen Shot 2023-10-18 at 5 37 00 pm Screen Shot 2023-10-18 at 5 36 51 pm

After change:

Screen Shot 2023-10-18 at 5 37 17 pm Screen Shot 2023-10-18 at 5 38 13 pm Screen Shot 2023-10-18 at 5 37 13 pm Screen Shot 2023-10-18 at 5 37 34 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.

Can you rebase to make sure it is not breaking any of the tests introduced earlier this morning?

Also please revert the changes to changelog.md

@TaranDev
Copy link
Author

Hi,

I've reverted changelog.md

When I rebase, I'm intermittently passing and failing 2 of the tests for workday-waiver:

Screen Shot 2023-10-19 at 9 13 33 am Screen Shot 2023-10-19 at 9 14 14 am

However the same happens on a fresh clone of the repo and the failing tests don't seem related to my changes, so I don't believe my code changes are the cause of the issue.

@araujoarthur0
Copy link
Collaborator

We can ignore these two tests, I'm fixing them in a different PR.

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #1020 (2b529d6) into main (8026942) will increase coverage by 2.10%.
Report is 20 commits behind head on main.
The diff coverage is 35.00%.

❗ Current head 2b529d6 differs from pull request most recent head 3c0ed37. Consider uploading reports for the commit 3c0ed37 to get more accurate results

@@            Coverage Diff             @@
##             main    #1020      +/-   ##
==========================================
+ Coverage   72.56%   74.67%   +2.10%     
==========================================
  Files          26       26              
  Lines        2180     2207      +27     
  Branches      345      352       +7     
==========================================
+ Hits         1582     1648      +66     
+ Misses        598      559      -39     
Files Coverage Δ
src/preferences.js 75.00% <35.00%> (-9.27%) ⬇️

... and 7 files with indirect coverage changes

Copy link
Collaborator

@araujoarthur0 araujoarthur0 left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this. Please take a look at my suggestions.

src/preferences.js Outdated Show resolved Hide resolved
src/preferences.js Outdated Show resolved Hide resolved
src/preferences.js Outdated Show resolved Hide resolved
src/preferences.js Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

When one inputs something wrong, the html message asks to follow the requested format, but that is not explicit.
Is there a way to customize that html message?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, consider if it's possible to implement extra tests for this behavior to ensure it's always working.

@TaranDev
Copy link
Author

TaranDev commented Nov 2, 2023

Thanks for the suggestions araujoarthur0, I'll probably start working on them when I'm free this weekend

@araujoarthur0
Copy link
Collaborator

Thanks for implementing the revisions @TaranDev!
Do you think it would be possible to add tests?
There's also a conflict with the package-lock.json. It's better to avoid committing changes to that file.

@thamara
Copy link
Owner

thamara commented Dec 27, 2023

Here are some tests I suggest:

describe('convertTimeFormat', () => {
    it('should convert single digit hour to HH:MM format', () => {
        expect(convertTimeFormat('6')).toBe('06:00');
    });

    it('should convert double digit hour to HH:MM format', () => {
        expect(convertTimeFormat('12')).toBe('12:00');
    });

    it('should convert H.MM format to HH:MM format', () => {
        expect(convertTimeFormat('6.5')).toBe('06:30');
    });

    it('should convert H.MM format to HH:MM format', () => {
        expect(convertTimeFormat('6.50')).toBe('06:30');
    });

    it('should convert HH.MM format to HH:MM format', () => {
        expect(convertTimeFormat('12.5')).toBe('12:30');
    });

    it('should convert H:MM format to HH:MM format', () => {
        expect(convertTimeFormat('6:30')).toBe('06:30');
    });

    it('should convert HH:MM format to HH:MM format', () => {
        expect(convertTimeFormat('12:30')).toBe('12:30');
    });
});

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.

Improve preferences interface
4 participants