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

feat: include timezone in hourly displays #6913

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jun 28, 2024

Description

Adds timezones to hourly time displays to reduce confusion in shared screenshots and the like.

Preview

On master:

SV:
image
EN:
image

On this branch:

SV:
image
EN:
image

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

Copy link
Contributor

@silkeholmebonnen silkeholmebonnen left a comment

Choose a reason for hiding this comment

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

Nice work! One optional suggestion:)

web/src/utils/formatting.test.ts Outdated Show resolved Hide resolved
@silkeholmebonnen
Copy link
Contributor

silkeholmebonnen commented Jul 8, 2024

I just noticed this! Can you create a follow up PR to fix it?
Or maybe it's a good time to move the dates on tooltips now? AVO-95
image

@VIKTORVAV99
Copy link
Member Author

I just noticed this! Can you create a follow up PR to fix it? Or maybe it's a good time to move the dates on tooltips now? AVO-95 image

Ah!
I'll pull the ripcord on AVO-95 and do that tomorrow.

@madsnedergaard
Copy link
Member

I think we should have been a bit more thorough on checking all cases where a date is present 😅

Screenshot 2024-07-16 at 20 36 13

@VIKTORVAV99
Copy link
Member Author

I think we should have been a bit more thorough on checking all cases where a date is present 😅

Screenshot 2024-07-16 at 20 36 13

This tooltip will soon get a revamp as well...
But yes we probably should have been more careful about this change, visually.

@madsnedergaard
Copy link
Member

Did you try/consider using date-fns or dayjs with AdvancedFormat to display dates in a shorter way?

@VIKTORVAV99
Copy link
Member Author

Did you try/consider using date-fns or dayjs with AdvancedFormat to display dates in a shorter way?

Since we don't use dayjs I'm not super keen on replacing every date-fns function with it. Cause having two date time libraries makes no sense. And with date-fns we need to import all the locales separately (it's built to be as tiny as possible) and I think they would format things roughly the same.

In theory we can build or own locale with Intl and control the format more tightly but I simply don't think it's worth it.

Especially since the tooltips themselves are the problem as they have not been migrated to the new design yet.

@madsnedergaard
Copy link
Member

Did you try/consider using date-fns or dayjs with AdvancedFormat to display dates in a shorter way?

Since we don't use dayjs I'm not super keen on replacing every date-fns function with it. Cause having two date time libraries makes no sense. And with date-fns we need to import all the locales separately (it's built to be as tiny as possible) and I think they would format things roughly the same.

In theory we can build or own locale with Intl and control the format more tightly but I simply don't think it's worth it.

Especially since the tooltips themselves are the problem as they have not been migrated to the new design yet.

I think it's a larger problem than just tooltips, but happy to discuss it separately at another time :)

This is on MacOS 14" with Arc:
Screenshot 2024-07-16 at 20 59 14

And this is in full-width on an external display:
Screenshot 2024-07-16 at 20 59 41

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.

3 participants