-
Notifications
You must be signed in to change notification settings - Fork 332
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
upcoming: [M3-7612] Placement Group Linodes List #10123
Conversation
159ad1a
to
d01b330
Compare
marginLeft: 0, | ||
padding: props.compact ? theme.spacing(5) : theme.spacing(10), | ||
width: '100%', | ||
})); |
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.
this was creating test console errors
Coverage Report: β
|
e905649
to
9ad8405
Compare
const { | ||
data: allLinodes, | ||
error: linodesError, | ||
isLoading: linodesLoading, | ||
} = useAllLinodesQuery(); |
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.
This isn't a big deal right now because sadly useAllLinodesQuery
is used in a ton of places, but we might want to consider loading only the necessary Linodes using a filter. We could also paginate and search with the API filter if we want to be extra.
const { | |
data: allLinodes, | |
error: linodesError, | |
isLoading: linodesLoading, | |
} = useAllLinodesQuery(); | |
const { | |
data: allLinodes, | |
error: linodesError, | |
isLoading: linodesLoading, | |
} = useAllLinodesQuery( | |
{}, | |
{ | |
'+or': placementGroup?.linode_ids.map((id) => ({ | |
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.
Sweet! Thx for suggesting those improvements - I made this change and updated the MSW linode/instances response to handle it. Hopefully helpful for other devs in the future π
...PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodesTable.tsx
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.
Thanks for all the test coverage to compliment these changes. Approving pending a couple small styling adjustments and some minor clean up.
Confirmed:
- Table and data looks good at all screen sizes.
- Table columns are sortable.
- Table is paginated when there are >25 PGs.
- Search bar filters table items.
...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx
Outdated
Show resolved
Hide resolved
packages/manager/.changeset/pr-10123-upcoming-features-1706733948755.md
Outdated
Show resolved
Hide resolved
...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx
Show resolved
Hide resolved
...PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.test.tsx
Outdated
Show resolved
Hide resolved
...PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodesTable.tsx
Outdated
Show resolved
Hide resolved
...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx
Outdated
Show resolved
Hide resolved
...ures/PlacementGroups/PlacementGroupsDetail/PlacementGroupsLinodes/PlacementGroupsLinodes.tsx
Show resolved
Hide resolved
3da098e
to
a323784
Compare
@@ -132,7 +136,7 @@ export const PlacementGroupsLanding = React.memo(() => { | |||
onButtonClick={handleCreatePlacementGroup} | |||
title="Placement Groups" | |||
/> | |||
<Typography sx={{ mb: 4, mt: 2 }}> | |||
<Typography sx={{ mb: 4, mt: 2, px: matchesSmDown ? 1 : 0 }}> |
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.
This is a neat way to style the component.
@carrillo-erik am not able to reproduce what you are seeing, but it may have to be with OSX scrollbar and not related to this PR, rather the LandingHeader. If you find it to be a consistent issue across entities, please make a ticket so it can be handled globally |
Description π
PR to implement the linode list on the Placement Group detail, using existing UI/UX patterns.
Changes π
Preview π·
How to test π§ͺ
Prerequisites
Verification steps
http://localhost:3000/placement-groups/1/linodes
As an Author I have considered π€
Check all that apply