-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
chore: fix visibility toggle in host filters #6976
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.
👍 Looks good to me! Reviewed everything up to 9711039 in 28 seconds
More details
- Looked at
73
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:4
- Draft comment:
The import ofButton
from 'antd' is new and necessary for the toggle button implementation. Ensure that this change is consistent with the rest of the UI components used in the project. - Reason this comment was not posted:
Confidence changes required:10%
The import ofButton
from 'antd' is new, and it is used in the toggle button for filters. This is a correct usage as per the PR description, which mentions fixing the toggle filters visibility button.
2. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:14
- Draft comment:
The import ofFilter
from 'lucide-react' is new and used for the filter toggle button. Ensure that this icon aligns with the design guidelines of the project. - Reason this comment was not posted:
Confidence changes required:10%
TheFilter
icon from 'lucide-react' is used in the new toggle button for filters. This is consistent with the PR's intent to fix the toggle filters visibility button.
3. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:78
- Draft comment:
The.hosts-list-table-header
class is newly added to style the header containing the toggle button and controls. Ensure this styling is consistent with the overall design. - Reason this comment was not posted:
Confidence changes required:10%
Thehosts-list-table-header
class is newly added to wrap the toggle button and controls. This is a structural change to accommodate the new toggle button.
4. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:84
- Draft comment:
Avoid using hardcoded color values like#25e192
and#62687c
. Use design tokens or predefined color constants instead. This applies to lines 100, 235, 236, 237, 241, 242, and 243. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_4j6zmu8X24ru0cNs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
d87b73f
to
69743a5
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! Incremental review on d87b73f in 31 seconds
More details
- Looked at
30
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/K8sFiltersSidePanel/K8sFiltersSidePanel.tsx:51
- Draft comment:
Consider adding a 'touchstart' event listener alongside 'mousedown' to improve mobile support. - Reason this comment was not posted:
Confidence changes required:50%
The useEffect hook for handling clicks outside the side panel is correctly implemented, but it should also handle touch events for better mobile support.
2. frontend/src/container/InfraMonitoringK8s/K8sFiltersSidePanel/K8sFiltersSidePanel.tsx:53
- Draft comment:
Consider adding a 'touchstart' event listener alongside 'mousedown' to improve mobile support. - Reason this comment was not posted:
Confidence changes required:50%
The useEffect hook for handling clicks outside the side panel is correctly implemented, but it should also handle touch events for better mobile support.
Workflow ID: wflow_faO0kFRuHH74o5mO
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Fix the toggle filters visibility button in hosts list
Related Issues / PR's
Screenshots
Affected Areas and Manually Tested Areas
Infra Monitoring
Important
Fixes filter visibility toggle in
HostsList.tsx
and adds click outside handler inK8sFiltersSidePanel.tsx
.HostsList.tsx
by adding a button to toggleshowFilters
state.K8sFiltersSidePanel.tsx
to close the panel when clicking outside.Button
component for filter toggle inHostsList.tsx
.InfraMonitoring.styles.scss
to support new toggle button layout.Filter
icon fromlucide-react
inHostsList.tsx
.This description was created by
for d87b73f. It will automatically update as commits are pushed.