-
Notifications
You must be signed in to change notification settings - Fork 361
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: [M3-6752] - Assign Linodes to Subnet drawer #9687
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.
- The file for the SubnetAssignDrawer itself... is pretty big 😅 -- I'll be looking into potentially breaking it down (maybe moving the form portion into a different file? idk for sure yet).
- There are quite a few similarities between this drawer and the Unassign Linodes one -- probably worth trying to cleanup/abstract more after both get merged in!
- After assigning a Linode to a subnet, if we go back to the Linode's page and delete it, the Linode will still appear to be assigned to the subnet (even though the Linode no longer exists) unless we refresh the page, because we don't invalidate any subnet/vpc related queries when deleting Linodes. We could potentially look into this link (ty Dajahi!) -- could be worth following up on this in a separate ticket?
@@ -2,12 +2,14 @@ import { SxProps } from '@mui/system'; | |||
import * as React from 'react'; | |||
import { CSVLink } from 'react-csv'; | |||
|
|||
import DownloadIcon from 'src/assets/icons/lke-download.svg'; |
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.
The changes in this file match @jaalah-akamai 's ticket for the Unassign Linode subnet drawer (copy-paste tbh), since they're super similar -- may have merge conflicts later
width: '100%', | ||
})); | ||
|
||
export const SelectedOptionsHeader = styled('h4', { |
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.
Similarly, a lot of the styling in this file matches/copies @jaalah-akamai 's ticket as the lists in the UX mockup are the same -- we could move this styling to a common styling file or have both drawers import their styling from here later!
@@ -0,0 +1,80 @@ | |||
import { Subnet } from '@linode/api-v4'; |
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.
Unit tests here are pretty basic tbh, will need to be a lot more thorough in the integration tests (see all the cases to test for 😭)
<Typography variant="body1">{REBOOT_LINODE_WARNING}</Typography> | ||
</DismissibleBanner> | ||
)} | ||
<VPCSubnetsTable vpcId={vpc.id} vpcRegion={vpc.region} /> |
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.
Adding vpc.region as a prop to the SubnetsTable because when determining which linodes we can assign to a subnet, we want them to be in the same region as the VPC
packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx
Outdated
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.
Went down the functionality checklist and everything LGTM ✅ 🚀
Going ahead and approving but I did leave a few small comments
packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.stories.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx
Outdated
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.
packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx
Show resolved
Hide resolved
packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx
Show resolved
Hide resolved
packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsList.tsx
Show resolved
Hide resolved
|
@coliu-akamai thx for confirming!
Let's change to "Number of available IP Address: {x}" to make it clearer cause it could look like a part of an ip address for instance |
Remove MUI warning from linodes autocomplete
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 great thanks for the changes and the follow up tasks - nice job! not an easy one 🚢
Description 📝
Add new drawer for assigning linodes
TODOS
Major Changes 🔄
Preview 📷
How to test 🧪
Test the following flows: (****integration tests for these flows will be handled in a separate ticket/pr!)
++anything else you can think of that I haven't mentioned here
Honestly, wouldn't recommend using the MSW and instead using just your dev account as it's hard to see changes/confirm all the error handling on it