-
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
chore: add functionality to open entity details from nested table items in groupby mode #6981
Conversation
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 06fd20e in 1 minute and 47 seconds
More details
- Looked at
458
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Clusters/K8sClustersList.tsx:359
- Draft comment:
Typo in function name:setselectedClusterName
should besetSelectedClusterName
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While camelCase naming with capital letters for each word is a common React convention, we can't verify if this is actually inconsistent with other files since we only have access to this one. The function is used consistently within this file. Making this change would require changes in multiple places in this file and potentially other files we can't see.
The comment could be correct about broader codebase consistency issues. The camelCase convention it suggests is indeed more standard in React.
However, we can't verify the broader codebase consistency claim. Within this file, the current naming is used consistently. Making this change would require coordinated changes across files we can't see.
Delete this comment. While it suggests a reasonable convention, we can't verify the cross-file consistency claims, and the current naming is at least internally consistent within this file.
2. frontend/src/container/InfraMonitoringK8s/DaemonSets/K8sDaemonSetsList.tsx:366
- Draft comment:
Typo in function name:setselectedDaemonSetUID
should besetSelectedDaemonSetUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/container/InfraMonitoringK8s/Deployments/K8sDeploymentsList.tsx:369
- Draft comment:
Typo in function name:setselectedDeploymentUID
should besetSelectedDeploymentUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/InfraMonitoringK8s/Jobs/K8sJobsList.tsx:342
- Draft comment:
Typo in function name:setselectedJobUID
should besetSelectedJobUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/InfraMonitoringK8s/Namespaces/K8sNamespacesList.tsx:366
- Draft comment:
Typo in function name:setselectedNamespaceUID
should besetSelectedNamespaceUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/InfraMonitoringK8s/Nodes/K8sNodesList.tsx:345
- Draft comment:
Typo in function name:setselectedNodeUID
should besetSelectedNodeUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/InfraMonitoringK8s/Pods/K8sPodLists.tsx:439
- Draft comment:
Typo in function name:setSelectedPodUID
should besetSelectedPodUID
for consistency. This typo appears in multiple files. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. frontend/src/container/InfraMonitoringK8s/Clusters/K8sClustersList.tsx:358
- Draft comment:
Avoid using inline styles in theonRow
function. Use CSS classes or styled components instead. This is also applicable in other files likeK8sDaemonSetsList.tsx
,K8sDeploymentsList.tsx
,K8sJobsList.tsx
,K8sNamespacesList.tsx
,K8sNodesList.tsx
,K8sPodLists.tsx
,K8sStatefulSetsList.tsx
, andK8sVolumesList.tsx
. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Eq71BEsD1mlJKpbq
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.
frontend/src/container/InfraMonitoringK8s/InfraMonitoringK8s.styles.scss
Show resolved
Hide resolved
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.
can we extract the following into a common custom hook and consume it in the components? we can have a single source of truth for these and whenever needed, we can make changes in a single place.
If merging this PR is urgent, please feel free to make the changes in a separate PR 🙂.
const nestedDaemonSetsData = useMemo(() => {
if (!selectedRowData || !groupedByRowData?.payload?.data.records) return [];
return groupedByRowData?.payload?.data?.records || [];
}, [groupedByRowData, selectedRowData]);
const selectedDaemonSetData = useMemo(() => {
if (groupBy.length > 0) {
// If grouped by, return the daemonset from the formatted grouped by data
return (
nestedDaemonSetsData.find(
(daemonSet) => daemonSet.daemonSetName === selectedDaemonSetUID,
) || null
);
}
// If not grouped by, return the daemonset from the daemonsets data
return (
daemonSetsData.find(
(daemonSet) => daemonSet.daemonSetName === selectedDaemonSetUID,
) || null
);
}, [
selectedDaemonSetUID,
groupBy.length,
daemonSetsData,
nestedDaemonSetsData,
]);
```
Yeah makes sense to move it to a single place. I've a few other changes to pick up this week, so merging this as it is for now. But will create an issue for it and address it in a later PR. Thanks! |
…ms in groupby mode
06fd20e
to
ab0110e
Compare
Summary
Add functionality to open the entity details modal from nested table items in groupby mode
Related Issues / PR's
Fixes #6883
Screenshots
NA
Affected Areas and Manually Tested Areas
K8s Infra Monitoring section
Important
Add functionality to open entity details from nested table items in groupby mode across Kubernetes monitoring components.
K8sClustersList.tsx
,K8sDaemonSetsList.tsx
, andK8sDeploymentsList.tsx
.onRow
click handlers to set selected entity UID and applyexpanded-clickable-row
class inK8sJobsList.tsx
,K8sNamespacesList.tsx
, andK8sNodesList.tsx
.nestedClustersData
,nestedDaemonSetsData
,nestedDeploymentsData
, etc., to handle nested data in groupby mode.selectedClusterData
,selectedDaemonSetData
,selectedDeploymentData
, etc., to fetch data from nested structures when grouped.expanded-clickable-row
class toInfraMonitoringK8s.styles.scss
for styling nested table rows.expandedRowRender
to includeonRow
click handlers for nested data inK8sPodLists.tsx
andK8sStatefulSetsList.tsx
.This description was created by for 06fd20e. It will automatically update as commits are pushed.