-
Notifications
You must be signed in to change notification settings - Fork 367
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-7610] - Placement Groups Detail #10096
Conversation
7d8f38e
to
23402d0
Compare
packages/manager/src/features/PlacementGroups/PlacementGroupsDetail/PlacementGroupsDetail.tsx
Outdated
Show resolved
Hide resolved
Coverage Report: ✅ |
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.
Looking good, tested all functionality. Left one technical suggestion.
packages/manager/src/features/PlacementGroups/PlacementGroupsLanding/PlacementGroupsRow.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/PlacementGroups/PlacementGroupsDetail/PlacementGroupsDetail.tsx
Outdated
Show resolved
Hide resolved
editableTextTitle: isEditingLabel | ||
? label | ||
: `${label} (${affinityLabel})`, |
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.
Suggestion: instead of storing isEditingLabel
in this component, the editableTextTitle
prop can accept an optional function (i.e., it would have type editableTextTitle: string | (isEditinglabel: boolean) => string
), which avoids duplicating state that is already tracked in the EditableText
component as isEditing
.
editableTextTitle: isEditingLabel | |
? label | |
: `${label} (${affinityLabel})`, | |
editableTextTitle: (isEditingLabel) => isEditingLabel | |
? label | |
: `${label} (${affinityLabel})`, |
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.
Yeah that's fair, though the solution above isn't too much of an improvement because of how the initial state of this component is managed in the first place.
I went the route of adding a new prop instead which bypasses relying on state all together. See 31c2f92
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.
Also added a test for the new prop and a story for good measure
className={classes.button} | ||
data-qa-save-edit | ||
onClick={finishEditing} | ||
> | ||
<Check className={classes.icon} /> | ||
</Button> | ||
<Button | ||
aria-label="Cancel" |
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.
improving accessibility while I am here. We shouldn't have buttons with only icons as children without an aria label
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.
Nice catch, I'll make sure to keep an eye for this type of detail.
f1e65fc
to
c29bb1b
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.
Thanks for resolving my concerns with the breadcrumb state. Re-tested and looks good; approved pending consideration of some minor changes.
packages/manager/src/features/PlacementGroups/PlacementGroupsDetail/PlacementGroupsDetail.tsx
Show resolved
Hide resolved
c29bb1b
to
66685e0
Compare
Description 📝
This PR implements the Placement Groups Detail page and associated routing. It does not implement tabs content (summary & linodes list) which are left for subsequent tickets.
Changes 🔄
EditableTextField
take a newtextSuffix
prop to manage the breadcrumb suffixPreview 📷
How to test 🧪
Verification steps
As an Author I have considered 🤔
Check all that apply