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

fix: detach the log indicator from timestamp column #6681

Merged
merged 2 commits into from
Dec 20, 2024

Conversation

vikrantgupta25
Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 commented Dec 19, 2024

Summary

  • detach the log state indicator from the timestamp column as it can be re-arranged according to need

Related Issues / PR's

contributes to #6679

Screenshots

Screen.Recording.2024-12-19.at.11.15.01.PM.mov

Affected Areas and Manually Tested Areas

@github-actions github-actions bot added the bug Something isn't working label Dec 19, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 15b8447 in 40 seconds

More details
  • Looked at 139 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/components/Logs/TableView/useTableView.tsx:82
  • Draft comment:
    Consider defining 'state-indicator' as a constant and using it throughout the code to avoid potential errors if the key needs to be changed in the future. This applies to other files as well where 'state-indicator' is used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses the 'state-indicator' key in multiple places to identify the log state indicator column. This is a good practice for consistency and clarity. However, the key is used as a string literal in multiple places, which can lead to errors if it needs to be changed in the future. It would be better to define it as a constant and use the constant throughout the code.
2. frontend/src/container/LogsExplorerList/InfinityTableView/styles.ts:28
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency in theming. This applies to other instances in this file as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/components/Logs/TableView/useTableView.styles.scss:22
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency in theming. This applies to other instances in this file as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_oUQq7M14OSMdqOF8


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@vikrantgupta25 vikrantgupta25 merged commit 8e7c78e into main Dec 20, 2024
15 of 16 checks passed
@vikrantgupta25 vikrantgupta25 deleted the detach-log-indicator-timestamp branch December 20, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LOGS[FE]: Severity colour shouldn't be tied to timestamp and should be independent.
3 participants