-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: truncate very long lines in traces explorer list view #6987
base: main
Are you sure you want to change the base?
Conversation
fc86338
to
f752251
Compare
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.
👍 Looks good to me! Reviewed everything up to fc86338 in 59 seconds
More details
- Looked at
67
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/periscope/components/LineClampedText/LineClampedText.tsx:38
- Draft comment:
The class nameline-clamped-wrapper__text
in the JSX file does not match the BEM convention used in the SCSS file. Consider usingline-clamped-wrapper__text
consistently in both files. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems confused - it says the class name doesn't match BEM convention but then suggests using the exact same name that's already there. Without seeing the SCSS file, we can't verify if there's actually a mismatch. The comment appears to be incorrect or at least poorly formed. The change itself seems to be improving BEM naming by making it more structured.
I haven't seen the SCSS file, so I can't be 100% certain about the naming conventions used there. The comment might be pointing to a real issue but expressing it poorly.
Even without seeing the SCSS file, the comment is self-contradictory and therefore not useful in its current form. The new class names follow BEM conventions better than the old ones.
Delete this comment as it's contradictory and doesn't provide clear, actionable feedback. The code change appears to be improving the BEM naming structure.
2. frontend/src/periscope/components/LineClampedText/LineClampedText.styles.scss:2
- Draft comment:
Themax-height: 300px;
might not be suitable for all text content. Consider making this value configurable or ensuring it aligns with the design requirements. - Reason this comment was not posted:
Confidence changes required:50%
Themax-height
of 300px in the SCSS file might not be appropriate for all use cases. It could lead to inconsistent UI behavior if the text content varies significantly in length.
3. frontend/src/periscope/components/LineClampedText/LineClampedText.tsx:39
- Draft comment:
Avoid using inline styles. Consider using CSS classes or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
4. frontend/src/container/TracesExplorer/ListView/utils.tsx:118
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for the Tag component here and elsewhere in the file. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_GhVZeSj7FcjGTRGR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 994b1c7 in 28 seconds
More details
- Looked at
130
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/PanelWrapper/__tests__/__snapshots__/TablePanelWrapper.test.tsx.snap:269
- Draft comment:
Snapshot update: Verify that the updated class name 'line-clamped-wrapper__text' is applied consistently for line truncation. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/container/PanelWrapper/__tests__/__snapshots__/TablePanelWrapper.test.tsx.snap:267
- Draft comment:
Renamed CSS class from 'line-clamped-text' to 'line-clamped-wrapper__text' for improved clarity. Ensure that all component implementations and styles referencing the old class have been updated consistently. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_m3lgZMoh3kP9hif3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related Issues / PR's
Close https://github.com/SigNoz/engineering-pod/issues/2147
Screenshots
Before:
2025-01-30.16-34-36.mov
After:
2025-01-30.16-35-06.mov
Affected Areas and Manually Tested Areas
Traces explorer list view
Important
Truncate long lines in traces explorer list view using
LineClampedText
with updated styles and tests.LineClampedText
inutils.tsx
.LineClampedText
component inLineClampedText.tsx
with overflow detection and tooltip.LineClampedText.styles.scss
for line clamping.TablePanelWrapper.test.tsx.snap
to reflect new class names and structure.This description was created by for 994b1c7. It will automatically update as commits are pushed.