Skip to content

Conversation

Substancia
Copy link

@Substancia Substancia commented Oct 2, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #2654

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

Set to 12-in Settings -> General

12-hours 24-hours
12-1 24-1
12-2 24-2
12-3 24-3

@Substancia
Copy link
Author

Hi @CommanderStorm , thanks for the feedback! Resolved all the tasks, can you please take another look and get this merged?

Reg. the comment on creating a radio button component, I'd be happy to take that up as a separate PR after this.

@Substancia Substancia changed the title Feature/time format feat: add support for 12-hour time format Oct 3, 2023
@Substancia Substancia marked this pull request as ready for review October 3, 2023 19:52
@Substancia
Copy link
Author

@CommanderStorm Seeing your approval, I have pulled the PR out of draft. Please merge this, TIA!

@Substancia
Copy link
Author

Looping in @louislam in case only they have write access.

Copy link
Collaborator

@chakflying chakflying left a comment

Choose a reason for hiding this comment

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

I think this is a good feature to have. However, having to fetch the setting from database every time the monitor details page loads is too inefficient. We should just fetch the setting once on initial load, and cache it in $root. The Settings page handling can stay the same.

@Substancia
Copy link
Author

Substancia commented Oct 8, 2023

@chakflying Thanks for the feedback! Made changes mentioned on the files.

We should just fetch the setting once on initial load, and cache it in $root.

How would this behave when a user goes to settings and change the time format and then come back to dashboard? Wouldn't the old format persist, since the data is fetched only during initial load and cached? Isn't the existing call a necessary expense?

@chakflying
Copy link
Collaborator

chakflying commented Oct 8, 2023

We can update the stored value after the user saves the Setting. Since it is stored in $root, it can then be used across the application.

@louislam louislam added this to the 2.0.0 milestone Oct 8, 2023
@Substancia
Copy link
Author

@louislam Can we please get this merged? TIA!

@chakflying
Copy link
Collaborator

The created lifecycle event runs when the current component is created. In a client-side routing SPA, this means it runs every time the Details page is loaded.

@louislam louislam added the question Further information is requested label Oct 12, 2023
@louislam louislam modified the milestones: 2.0.0, 2.1.0 Nov 23, 2023
@CommanderStorm CommanderStorm added the A:dashboard Issues or PRs related to the main dashboard page where monitors' status are shown label Dec 5, 2023
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 19, 2024
@github-actions github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label May 19, 2024
@github-actions github-actions bot removed the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label May 19, 2024
@CommanderStorm
Copy link
Collaborator

The created lifecycle event runs when the current component is created. In a client-side routing SPA, this means it runs every time the Details page is loaded.

@chakflying What lifecycle event/.. would you recomend instead? created seems the earliest event avaliable to the options api and feels like the correct one for loading settings..

@CommanderStorm CommanderStorm removed the pr:please address review comments this PR needs a bit more work to be mergable label Aug 24, 2024
@AJolly
Copy link

AJolly commented Oct 18, 2024

This would be great to get this merged in!

@Skyedra
Copy link

Skyedra commented Feb 11, 2025

Would love to see this get revived! 24-hour time is cumbersome to read for people in regions where it is not used, so this feature would be appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:dashboard Issues or PRs related to the main dashboard page where monitors' status are shown needs:resolve-merge-conflict question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

time format setting in 12 and 24 format

6 participants