DI-23223: Update the EditAlertResources component to save the added / removed resources and Show only selected resources #11613
Conversation
venkymano-akamai
changed the title
venkymano-akamai
changed the title
venkymano-akamai
changed the title
| export const isResourcesEqual = ( | ||
| originalResourceIds: string[] | undefined, | ||
| selectedResourceIds: string[] | ||
| ): boolean => { | ||
| if (!originalResourceIds) { | ||
| return selectedResourceIds.length === 0; | ||
| } | ||
|
|
||
| if (originalResourceIds?.length !== selectedResourceIds.length) { | ||
| return false; | ||
| } | ||
|
|
||
| const originalSet = new Set(originalResourceIds); | ||
| return selectedResourceIds.every((id) => originalSet.has(id)); | ||
| }; |
There was a problem hiding this comment.
@venkymano-akamai can we re-use functions from dashboards for cases like this? ex - deepequal func ..
There was a problem hiding this comment.
But here we don't need deep equals, we need deep equals in case of comparing complex objects, here it is primitive type, we can just use what we have
| }, | ||
| })} | ||
| data-testid="show_selected_only" | ||
| disabled={!(Boolean(selectedResources.length) || selectedOnly)} |
There was a problem hiding this comment.
no need Boolean function
| disabled={!(Boolean(selectedResources.length) || selectedOnly)} | |
| disabled={!selectedResources.length || selectedOnly} |
|
|
||
| const { mutateAsync: editAlert } = useEditAlertDefinition( | ||
| serviceType, | ||
| Number(alertId) |
There was a problem hiding this comment.
is it necessary to send it as number? can't it go just as string?
|
@nikhagra-akamai , Thanks for the approval, addressed your comments as well. |
|
Coverage Report: ✅ |
venkymano-akamai
requested review from
pmakode-akamai
and removed request for
pmakode-akamai
| <Grid container spacing={3}> | ||
| <Grid columnSpacing={1} container item rowSpacing={3} xs={12}> | ||
| <Grid columnSpacing={2} container item rowSpacing={3} xs={12}> |
There was a problem hiding this comment.
I think alignItems="center" would benefit things here if we remove some of the checkbox styling.
| maxHeight: '34px', | ||
| pt: theme.spacing(1.2), | ||
| svg: { | ||
| backgroundColor: theme.color.white, |
There was a problem hiding this comment.
| backgroundColor: theme.color.white, | |
| backgroundColor: theme.tokens.color.Neutrals.White, |
| maxHeight: '34px', | ||
| pt: theme.spacing(1.2), |
There was a problem hiding this comment.
We should remove both of these. The alignItems of the parent should center things
| marginLeft: theme.spacing(1), | ||
| minHeight: 'auto', | ||
| minWidth: 'auto', |
There was a problem hiding this comment.
We can remove this entire styled component in place of StyledLinkButton which you can import from linode/ui
| alignItems: 'center', | ||
| background: theme.bg.bgPaper, | ||
| borderRadius: 1, | ||
| display: 'flex', | ||
| flexWrap: 'nowrap', | ||
| height: '54px', | ||
| marginBottom: 0, | ||
| padding: theme.spacing(2), |
There was a problem hiding this comment.
I would see how many of these we can remove, some of these are redundant and already provided by the Notice component.
| borderRadius: 1, | ||
| display: 'flex', | ||
| flexWrap: 'nowrap', | ||
| height: '54px', |
There was a problem hiding this comment.
Let's remove the height and ensure we have the proper padding
| sx={{ | ||
| ml: 1, | ||
| }} |
There was a problem hiding this comment.
Better to apply gap={1} display="flex" to the containing Box
| open={openConfirmationDialog} | ||
| title="Confirm alert updates" | ||
| > | ||
| <Typography fontSize="16px" variant="body1"> |
There was a problem hiding this comment.
There is no 16px token as depicted in the design, this is going to have to be 14px unless they want to add a new token
| <Typography fontSize="16px" variant="body1"> | |
| <Typography sx={(theme) => ({ | |
| font: theme.tokens.typography.Body, | |
| })} variant="body1"> |
| </StyledButton> | ||
| )} | ||
| {!isSelectAll && ( | ||
| <StyledButton |
| const isSelectAll = selectedResources !== totalResources; | ||
|
|
||
| return ( | ||
| <StyledNotice variant="info"> |
There was a problem hiding this comment.
Add gap={1} for the spacing you need between text and button
jaalah-akamai
added
Approved
Cloud Manager UI test results🎉 500 passing tests on test run #25 ↗︎
|
|
Merging since tests passed, lint passed, jobs passed and we have enough approvals |
Cloud Manager E2E
|
||||||||||||||||||||||||||||
| Project |
Cloud Manager E2E
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 30m 34s |
| Commit |
|
| Committer | venkatmano-akamai |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
500
|
| View all changes introduced in this branch ↗︎ | |
Description 📝
Update the EditAlertResources component to save the added / removed resources.
Changes 🔄
Target release date 🗓️
11-02-2025
Preview 📷
How to test 🧪
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅