Repository navigation
Conversation
|
Currently, this is far too much code for such a feature. It spreads too much. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2631 +/- ##
==========================================
- Coverage 90.84% 90.83% -0.01%
==========================================
Files 250 250
Lines 14403 14403
==========================================
- Hits 13084 13083 -1
- Misses 1319 1320 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…c to useTimezone composable - Deleted TimezoneProvider.vue and its associated logic. - Introduced useTimezone composable to manage timezone settings and provide necessary functions. - Updated various components and views to utilize the new useTimezone composable for timezone management. - Removed outdated timezone utility functions and tests, consolidating functionality within the new composable. - Adjusted data handling in graphs to directly use timestamps without shifting for timezone. - Enhanced tooltip formatting for time display based on selected timezone.
|
The change size is now OK. Thanks!
|
|
Let me work on it to add the missing parts |
- Shorten timezone selector label to "Browser time" to avoid two-line wrapping - Support timezone-aware start and end time inputs with automatic conversion on timezone change - Format X-axis pointer crosshair label according to the selected timezone - Improve X-axis tick formatting with multi-level awareness and midnight-to-day conversion - Ensure consistent 24-hour time formatting in the request summary header - Add comprehensive test coverage for timezone parsing, formatting, and conversion helpers
vincentbernat
left a comment
vincentbernat
left a comment
There was a problem hiding this comment.
The change now looks quite big and "brittle". It's difficult to review and I think it will be difficult to maintain properly.
For Sugar, it seems there is newDateInternal that could help, but I am not 100% sure it would cover all the cases.
For Echarts, there is issue apache/echarts#14453 and proposal apache/echarts#21739. If it helps, we could patch Echarts.
Alternatively, we could patch "Date" instead. This does not look trivial, but the change would be "contained".
|
|
||
| // Pre-computed list of timezone options | ||
| export const timezones: TimezoneItem[] = [ | ||
| { id: 1, code: "browser", name: `Browser Time (${browserTimezone})` }, |
There was a problem hiding this comment.
This makes the field big for most users. Just Browser time.
| const trimmed = str.trim(); | ||
|
|
||
| // If already ends with Z or has timezone offset (+/-HH:MM), native Date handles it | ||
| if (/[zZ]|[+-]\d{2}(:?\d{2})?$/.test(trimmed)) { |
There was a problem hiding this comment.
This also matches "2026-10-07".
| humanStart: start.toISOString(), | ||
| humanEnd: end.toISOString(), | ||
| humanStart: formatTimezoneDateTime(start), | ||
| humanEnd: formatTimezoneDateTime(end), |
There was a problem hiding this comment.
Not sure if this is useful. When you share a link and your time zones don't match, you get the wrong values.
| }; | ||
|
|
||
| export const formatTimezoneAxisPointer = (val: string | number): string => { | ||
| return formatTimezoneDateTime(val); |
There was a problem hiding this comment.
Not sure this function is useful if it's just an alias?
| return formatTimezoneDateTime(val); | ||
| }; | ||
|
|
||
| export const timezoneAxisFormatter = ( |
There was a problem hiding this comment.
I see three concerns:
- the function is pretty big (and the code looks like a bit redundant)
- no ticks will match midnight if there is a timezone like +07:30
- we end up with several ticks for the same day (Paris, we have the day at 00:00 UTC as well as for 22:00 UTC).
3 participants
Curently the timezone is the browser timezone but in some cases when you work with a team in multiple timezones it is required to have a common timezone.
This option mimics similar option in Grafana