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

fix: Customize 24h vs 12h time formatting globally #63

Merged
merged 18 commits into from Nov 30, 2023

Conversation

treypisano
Copy link
Contributor

What does this Pull Request Do?

Opening up this PR in order to discuss implementation of 12h and 24h time formats. I added the tokens used for formatting (ex: MMM d h:mm:ss a) as constant in utils.ts, in order to avoid copy and pasting these in all the places its needed.

Then, I used the userUserPreferences hook to read what the preference is in the Context, get the required token, and then format the graph accordingly.

At the moment, all the line charts and the logger are changing when the initialState is changed. Of course in the future this will be changed with a UI component, but I wanted to make sure the base implementation is solid first.

How did I test this?

  1. Launch the dev build using docker compose
  2. Open up either the logger or dashboard
  3. Change initial state from 24hr to 12hr , refresh, and observe how it changes

@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2023

🦋 Changeset detected

Latest commit: 84c21d7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/api Minor
@hyperdx/app Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

MikeShi42
MikeShi42 previously approved these changes Oct 29, 2023
Copy link
Contributor

@MikeShi42 MikeShi42 left a comment

Choose a reason for hiding this comment

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

really like the approach and a great start to standardizing how we do time formatting across the app 🙌


import { semanticKeyedColor, truncateMiddle } from './utils';
import Link from 'next/link';


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra new line

@@ -37,7 +37,7 @@ export default function SearchTimeRangePicker({
setInputValue,
onSearch,
showLive = false,
timeFormat = '24h',
timeFormat = '12h',
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing - it looks like we end up overwriting this in the UI via

const formatDate = (
(so after you hit search for the time range, we end up showing 24h time again instead of 12h)

That being said, I don't think it's a huge deal, and something we can probably clean up once #75 lands since it might conflict anyways.

@kodiakhq
Copy link

kodiakhq bot commented Oct 29, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@MikeShi42
Copy link
Contributor

Oh one more thing, could you add a changeset as well? @treypisano :)

feel free to ping me on discord as well if you need another review to get this merged before EoM :P (apologies I took forever on this one)

MikeShi42
MikeShi42 previously approved these changes Nov 18, 2023
Copy link
Contributor

@MikeShi42 MikeShi42 left a comment

Choose a reason for hiding this comment

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

just a minor copy nit. It looks like there's a few conflicts and also I think there's probably going to be some prettier/linter things to run through as well.

@@ -421,6 +428,17 @@ export default function TeamPage() {
</Modal.Body>
</Modal>
</div>
<div>
<h2 className="mt-5">Time Format</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we don't have a global personal setting place, could we maybe for now just put some subtext describing that this setting is a local setting and won't propagate across the entire team? Since this settings page is technically supposed to be the team page.

Something like:

<div className="text-muted my-2">
  Note: Only affects your own view and does not propagate to
  other team members.
</div>

Don't want to block this PR on how we should be treating personal vs team settings :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll implement this

@MikeShi42
Copy link
Contributor

@treypisano looks like just one more lint issue then we can get this merged :)

Copy link
Contributor

@MikeShi42 MikeShi42 left a comment

Choose a reason for hiding this comment

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

🚢 thank you!

@kodiakhq kodiakhq bot merged commit e8c26d8 into hyperdxio:main Nov 30, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants