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.

Sign up for GitHub

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

Jump to bottom

Allow first day of week to be Sunday or Monday #3093

Merged
merged 8 commits into from Oct 19, 2022
Merged

Allow first day of week to be Sunday or Monday #3093

merged 8 commits into from Oct 19, 2022

Conversation

Git-Jiro
Copy link
Contributor

@Git-Jiro Git-Jiro commented Oct 6, 2022 •
edited

refers to #3073

@Git-Jiro Git-Jiro force-pushed the change_startofweek branch 2 times, most recently from 208d3f9 to bd8ba85 Compare October 6, 2022 18:52
@Git-Jiro
Copy link
Contributor Author

@simonpasquier
who can give me feedback on this implementation of "choose week to start at monday or sunday"

@simonpasquier
Copy link
Member

@Git-Jiro thanks for doing this! I'm not really qualified for reviewing the code, @w0rm is definitely more qualified :)

For the other reviewers, this PR introduces a new "Settings" tab which allows to select in a drop-down list the first day of the week (Monday or Sunday):

image

From a feature standpoint, I wonder if the list shouldn't contain all week days?
Some padding between "Start of week:" and the drop-down list would be neat. It might also be good to mention in the Settings page that preferences are stored in the client's local storage (hence not shared between different clients).

@Git-Jiro
Copy link
Contributor Author

@simonpasquier good points!
regarding "put all the weekdays in the dropdown": That might imply that setting the other days as "first day of week" works as well. Unfortunately, "first day of week" only works for Monday and Sunday at the moment.

@Git-Jiro
Copy link
Contributor Author

Tried to make the settings page a little bit more visually appealing

@Git-Jiro
Allow week to start at Monday or Sunday
e07cdfa 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
w0rm
w0rm reviewed Oct 14, 2022
View reviewed changes
ui/app/src/Main.elm Outdated Show resolved Hide resolved
w0rm
w0rm reviewed Oct 14, 2022
View reviewed changes
ui/app/index.html Outdated Show resolved Hide resolved
@w0rm
Copy link
Member

w0rm commented Oct 14, 2022 •
edited

@Git-Jiro thanks for the contribution! I left a few comments.

@simonpasquier we shouldn't put all days in the dropdown, it's Monday or Sunday. Perhaps we could switch to the radio buttons, so that the user doesn't have to click twice to select the option?

@Git-Jiro
refactor from Int to DataType
4c6e34a 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@Git-Jiro
refactor to use name 'firstDayOfWeek' everywhere
8c04efd 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@Git-Jiro
CSS fine tuning
3f48186 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@Git-Jiro
Check firstDayOfWeek with case statements
5cb449a 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@Git-Jiro
Copy link
Contributor Author

For me it looks like the current test failure is a false positive, is it possible to restart the test?

@w0rm
Copy link
Member

w0rm commented Oct 17, 2022

@Git-Jiro thanks for the hard work! I have one last suggestion for the design. Let me know what you think. I am also happy to do it myself if you don't have time.

w0rm
w0rm reviewed Oct 17, 2022
View reviewed changes
ui/app/src/Main.elm Outdated Show resolved Hide resolved
@Git-Jiro
Copy link
Contributor Author

@Git-Jiro thanks for the hard work! I have one last suggestion for the design. Let me know what you think. I am also happy to do it myself if you don't have time.

I guess we are talking about radio buttons instead of the dropdown?
should not be that hard.

@Git-Jiro
Copy link
Contributor Author

Now with radio buttons:
Radio Buttons

@Git-Jiro
Change default to Sunday. Use radio buttons
6319577 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@Git-Jiro @w0rm
Update ui/app/src/Views/Settings/Views.elm
579254e 
Co-authored-by: Andrey Kuzmin <unsoundscapes@gmail.com>
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@w0rm
Copy link
Member

w0rm commented Oct 19, 2022

@Git-Jiro looks like assets_vfsdata.go needs to be regenerated. And then it is good to go! 🙏

@Git-Jiro
Regenerate assets_vfsdata.go
514278d 
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
@w0rm w0rm merged commit 893ad67 into prometheus:main Oct 19, 2022
@w0rm
Copy link
Member

w0rm commented Oct 19, 2022

@Git-Jiro thanks for your contribution. As a person living in Europe I definitely find this feature very useful 😁

@w0rm w0rm mentioned this pull request Oct 19, 2022
Closed
@Git-Jiro Git-Jiro deleted the change_startofweek branch October 19, 2022 16:03
qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Nov 16, 2022
@Git-Jiro @w0rm @qinxx108
Allow first day of week to be Sunday or Monday (prometheus#3093)
7492965 
* Allow week to start at Monday or Sunday

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* refactor from Int to DataType

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* refactor to use name 'firstDayOfWeek' everywhere

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* CSS fine tuning

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Check firstDayOfWeek with case statements

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Change default to Sunday. Use radio buttons

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Update ui/app/src/Views/Settings/Views.elm

Co-authored-by: Andrey Kuzmin <unsoundscapes@gmail.com>
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Regenerate assets_vfsdata.go

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
Co-authored-by: Andrey Kuzmin <unsoundscapes@gmail.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Dec 13, 2022
@Git-Jiro @w0rm @qinxx108
Allow first day of week to be Sunday or Monday (prometheus#3093)
d2a1ca4 
* Allow week to start at Monday or Sunday

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* refactor from Int to DataType

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* refactor to use name 'firstDayOfWeek' everywhere

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* CSS fine tuning

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Check firstDayOfWeek with case statements

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Change default to Sunday. Use radio buttons

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Update ui/app/src/Views/Settings/Views.elm

Co-authored-by: Andrey Kuzmin <unsoundscapes@gmail.com>
Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

* Regenerate assets_vfsdata.go

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>

Signed-off-by: Martin Schimandl <martin.schimandl@gmail.com>
Co-authored-by: Andrey Kuzmin <unsoundscapes@gmail.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@w0rm w0rm w0rm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Git-Jiro @simonpasquier @w0rm