-
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: added map view tab and coming soon message for the map detail view #6404
base: host-lists
Are you sure you want to change the base?
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> |
1 similar comment
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 b19164b in 56 seconds
More details
- Looked at
290
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/utils.tsx:37
- Draft comment:
Consider using a more descriptive key for the 'Map View' tab instead ofPANEL_TYPES.TIME_SERIES
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a code quality improvement by recommending a more descriptive key. However, without knowing the context ofPANEL_TYPES.TIME_SERIES
, it's hard to judge if this is necessary. The key might be part of a larger system whereTIME_SERIES
is appropriate. The comment is speculative without strong evidence that a change is required.
The comment might be valid ifPANEL_TYPES.TIME_SERIES
is indeed misleading or incorrect for a 'Map View'. However, without additional context, it's speculative.
Given the lack of context, it's safer to assume the current key is appropriate unless there's clear evidence otherwise.
Delete the comment as it is speculative and lacks strong evidence that a change is required.
2. frontend/src/pages/InfraMonitoringHosts/index.tsx:2
- Draft comment:
Avoid using thecomponent/index.tsx
file 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 on unchanged code.
Workflow ID: wflow_IvZeHlwxRhUQlRKQ
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.
items={getTabsItems()} | ||
className="infra-monitoring-tabs" | ||
type="card" | ||
defaultActiveKey={activeTab} |
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.
defaultActiveKey
is redundant when activeKey
is controlled by state. Consider removing it.
defaultActiveKey={activeTab} |
Summary
Related Issues / PR's
Screenshots
Affected Areas and Manually Tested Areas