feat: add timezone support to logs explorer chart (built with Chartjs) #6566
Conversation
…zone picker by clicking outside
…gs explorer -> list view)
…logs explorer -> list view)
… (logs explorer -> list view)
…of alert history page
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 612dd06 in 1 minute and 6 seconds
More details
- Looked at
145lines of code in3files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/components/Graph/utils.ts:2
- Draft comment:
Remove the import of 'chartjs-adapter-date-fns' as it is not used and 'chartjs-adapter-dayjs-3' is being used instead. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/components/Graph/index.tsx:130
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_azZrvepCaam4Neh8
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c4bfe81 in 11 seconds
More details
- Looked at
21lines of code in1files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/package.json:62
- Draft comment:
Thechartjs-adapter-dayjs-3dependency is missing. Please add it to thedependenciessection. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/package.json:62
- Draft comment:
Thechartjs-adapter-dayjs-3dependency is missing frompackage.json. Please add it to ensure the timezone support functionality works as intended. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_wTFu0G7pz2ZxSBDp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
ahmadshaheer
force-pushed
the
feat/apply-timezone-to-logs-explorer-chartjs-based-chart
branch
from
c4bfe81
to
fc88e02
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on fc88e02 in 43 seconds
More details
- Looked at
123lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/components/Graph/utils.ts:2
- Draft comment:
The import of 'chartjs-adapter-date-fns' is unnecessary since 'dayjs' is being used for date handling. Consider removing this import to avoid confusion. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/components/Graph/index.tsx:130
- Draft comment:
Avoid using hardcoded color values likergba(231,233,237,0.8). Use design tokens or predefined color constants instead. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_JvRYETVfkH0N9FqJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ahmadshaheer
force-pushed
the
feat/apply-timezone-to-tabular-data-throughout-the-app
branch
from
a7da3f3
to
0a21911
Compare
frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts
Outdated
Show resolved
Hide resolved
frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 44a3f1b in 23 seconds
More details
- Looked at
52lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts:78
- Draft comment:
userTimezoneis not optional in the function signature, so using optional chaining (?.) is unnecessary. Consider removing it for clarity and consistency. This also applies to line 63. - Reason this comment was not posted:
Confidence changes required:50%
The code is mostly well-structured, but there is a minor issue with the use of optional chaining on a non-optional property.
2. frontend/src/hooks/useTimezoneFormatter/useTimezoneFormatter.ts:39
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_GlVTSJsgD3OoWbI6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2b4b0ed in 37 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_5BtQLLsgHppXuXEV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
… into feat/apply-timezone-to-logs-explorer-chartjs-based-chart
ahmadshaheer
merged commit a0a611c
into
feat/apply-timezone-to-tabular-data-throughout-the-app
Summary
Related Issues / PR's
Part of https://github.com/SigNoz/engineering-pod/issues/2005
Screenshots
2024-12-01.22-26-13.mov
Affected Areas and Manually Tested Areas
Important
Adds timezone support across the application using a new
useTimezoneFormatterhook for consistent timestamp formatting.useTimezoneFormatterhook for consistent timezone-based timestamp formatting.TimezoneProviderto includeformatTimezoneAdjustedTimestampfunction.useTimezone.CustomTimePicker,RangePickerModal,Graph,ListLogView,RawLogView,TableView,LogsPanelComponent,TracesTableComponent,NoFilterTable,SaveView, and others to use timezone formatting.getGraphOptionsinutils.tsto format tooltips with timezone.useTimezoneFormatterto optimize repeated timestamp formatting.This description was created by
for 2b4b0ed. It will automatically update as commits are pushed.