-
Notifications
You must be signed in to change notification settings - Fork 963
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
chore(date formatting): clean up date formatting in the app #7584
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (2)
- web/src/utils/snapshots/formatting.test.ts.snap: Language not supported
- web/src/components/Time.tsx: Evaluated as low risk
Comments suppressed due to low confidence (1)
web/src/utils/formatting.ts:185
- The new 'formatDate' function, especially the 'isTimeHeader' parameter, should be covered by tests to ensure all cases are handled correctly.
const formatDate = (
While this is technically more correct, I wonder if this impacts the simplicity of the current user experience. I personally need to think a bit more about it and probably show it around to a few people to get some feedback before I can give proper feedback. PS: In case you're wondering if this is a departure from what we did with the x-ticks on the area charts: there I think this was necessary because for e.g. hourly values, it didn't make sense to have the midnight line start at the middle of the hour 00:00 (it should start at beginning of the value for the hour 00:00). Will think about it more. You just opened pandora's box! 😁 |
Haha I'm fine with just opening the discussion and closing this PR if we decide against it. But I did this after the feedback we got on LinkedIn in relation to not knowing if the data was for a single point in time or if it was for a period. While it's implicit for us (and probably other people working with time series data often) I'm not sure everyone would find it clear. I agree for yearly tho but that can be adapted anc easily changed to only say 2024. But talk to a few people, see what they think and then we can take it from there! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (10)
- web/src/utils/snapshots/formatting.test.ts.snap: Language not supported
- web/src/features/charts/tooltips/PriceChartTooltip.tsx: Evaluated as low risk
- web/src/utils/formatting.ts: Evaluated as low risk
- web/src/utils/formatting.test.ts: Evaluated as low risk
- web/src/utils/constants.ts: Evaluated as low risk
- web/src/features/time/TimeHeader.tsx: Evaluated as low risk
- web/src/features/time/HistoricalTimeHeader.tsx: Evaluated as low risk
- web/src/features/charts/tooltips/CarbonChartTooltip.tsx: Evaluated as low risk
- web/src/features/charts/tooltips/EmissionChartTooltip.tsx: Evaluated as low risk
- web/src/features/charts/tooltips/NetExchangeChartTooltip.tsx: Evaluated as low risk
Comments suppressed due to low confidence (1)
web/src/features/charts/tooltips/BreakdownChartTooltip.tsx:106
- Ensure that the new date formatting logic is covered by tests.
const displayByEmissions = useAtomValue(displayByEmissionsAtom);
Fixed the type issues so you can do some side by side comparisons! |
I've done a bit more thinking here. From a user perspective, we're having two different situations:
From a user perspective, I currently think an intuitive way to talk about 1. is to say "At XX:XX, X GW were produced using X".
In all cases, we never have to rely on showcasing two values (e.g. "01:00-02:00"), and I believe the language of "at" and "during" is both simple and precise. An important note is that the formatting depends on the frequency of the data (hourly, daily, monthly..) and not the window selected (24h, 72h, ...). An outstanding question is whether we deem hourly data to be an instant (category 1) or an interval (category 2). I am slightly tempted to categorise it into an interval (category 2), which would mean using "During 05:00", and the unit suffix "Wh". It would bring consistency to the whole app and simplify the code. On the flip side, it would make it harder for us when we add 5min or 15min data, as I assume these would classify as instants (category 1). |
I'll see what we can do about the formatting but we use the Intl standard and default JS options for this to ensure it's localized not only by language but by region as well. And because dates are what they are it's vastly different in the US vs say Europe. But I'll get back to you about what is possible! |
I think the idea here is more correct but we should ask ourselves "are we spending our time solving a problem that brings significant value to our users?". I've already spent time digging into all the total energy change PR's so I can bring more informed input to this discussion and I'm sure you all have too. We would also like to simplify the "data overload" present in the tooltips and adding more characters to them should be considered as a trade-off here of simplicity vs correctness. I think the "during" approach makes sense to but does bring some challenges if we move to 5 & 15 minute data. It could be best to park this discussion and tackle it in a holistic way (simple, consistent and correct) when we shift to finer granularity? |
Issue
It's not clear if the data is for a single point in time or for a range of time.
Description
Switches to use a date range for the date formatting and uses date fns functions instead of get/set UTC date all the time.
Preview
Double check
pnpx prettier@2 --write .
andpoetry run format
in the top level directory to format my changes.