-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: improve empty hosts, incorrect metrics and no filter views #6530
Conversation
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> |
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.
❌ Changes requested. Reviewed everything up to b5d10a8 in 1 minute and 10 seconds
More details
- Looked at
421
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/HostsEmptyOrIncorrectMetrics.tsx:13
- Draft comment:
Consider importing the image or using a configuration for image paths instead of hardcoding the path. This will make the code more maintainable and less prone to errors if the path changes. - Reason this comment was not posted:
Confidence changes required:50%
TheHostsEmptyOrIncorrectMetrics
component uses an image with a hardcoded path. This can lead to issues if the path changes or if the image is not available. It's better to use a more dynamic approach, such as importing the image or using a configuration for paths.
2. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:160
- Draft comment:
Consider refactoring the return statement to simplify the conditional rendering logic. This will improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:40%
TheHostsList
component has a complex return statement with multiple conditional renderings. This can be simplified for better readability and maintainability.
3. frontend/src/container/SideNav/NavItem/NavItem.tsx:42
- Draft comment:
Consider using a more descriptive class name for the 'New' tag styling to improve code clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:30%
TheNavItem
component uses a class namesidenav-new-tag
for styling the 'New' tag. This class name is not very descriptive and could be improved for better clarity.
Workflow ID: wflow_9mpxihJ0X4D9j2Tu
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.
b5d10a8
to
e2a4641
Compare
* feat: add usasge events * feat: add reqrest early access events
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.
Verified
- messaging when there is no data
- messaging when there is incorrect k8s data
- messaging for expressing interest
Important
Enhance infrastructure monitoring UI with new empty states, loading indicators, and highlight new features in the sidebar.
sentAnyHostMetricsData
andisSendingK8SAgentMetrics
toHostListResponse
ingetHostLists.ts
.HostsEmptyOrIncorrectMetrics
to display messages for no data or incorrect data states.HostsList
to handle new states and display appropriate messages or loading indicators.isNew
property toSidebarItem
and updateNavItem
to display "New" tag.menuItems.tsx
to mark "Infra Monitoring" as new.InfraMonitoring.styles.scss
.SideNav.styles.scss
to support "New" tag display.This description was created by for b5d10a8. It will automatically update as commits are pushed.