-
Notifications
You must be signed in to change notification settings - Fork 364
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-8855] - Surface Node Pool Tags #11368
base: develop
Are you sure you want to change the base?
feat: [M3-8855] - Surface Node Pool Tags #11368
Conversation
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.
Change looks good on desktop! I am wondering how we can best handle mobile view.
<Box | ||
alignItems="center" | ||
data-testid={encryptionStatusTestId} | ||
display="flex" | ||
> | ||
<Typography>Pool ID {poolId}</Typography> | ||
<StyledVerticalDivider /> | ||
<EncryptedStatus | ||
encryptionStatus={encryptionStatus} | ||
tooltipText={DISK_ENCRYPTION_NODE_POOL_GUIDANCE_COPY} | ||
/> | ||
</Box> | ||
) : ( | ||
<Typography>Pool ID {poolId}</Typography> | ||
)} | ||
</Box> |
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.
mobile.mov
We'll want to consider mobile view here, and might need some UX input. (Should we still have the ability to see tags in this view?) Currently, the tag cell will overlap with the pool id/encryption box once we get to ~500px in the viewport, and it gets worse at smaller screens (iPhone SE). Adjusting the box's width of 100% to less helps, but the tooltip still gets hidden. Maybe displaying tags under the pool id and encryption status is most readable?
Looks like there is also an existing issue where the Unlock icon has a min width of 16px, but the Lock icon does not, leading it to shrink and virtually disappear (and leave some odd spacing) behind at the smaller screen sizes. We could fix that here.
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.
@mjac0bs Good catches, should be addressed now. I moved the tags to a separate row on mobile and removed the minWidth of 16px
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 better! I think we should actually do this for tablet screen sizes too. There's an edge case with really long tags where, between tablet and mobile breakpoints, we still get some overlapping. Switching to displaying the tags below at the md
breakpoint is also consistent with the cluster's tags. (Which seem to have their own buggy display issue... I'll create a follow up ticket to fix cluster tags. Update: M3-9009)
No issues after using md
breakpoints instead of sm
:
Screen.Recording.2024-12-11.at.12.59.02.PM.mov
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! Approving pending my one comment in the smaller screen sizes thread.
Cloud Manager UI test results🎉 466 passing tests on test run #12 ↗︎
|
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.
Description 📝
The /pools endpoint returns Tags for node pools, but we don't currently surface them in the UI: https://techdocs.akamai.com/linode-api/reference/get-lke-node-pool
At the bottom of each node pool section, we will now display the list of tags associated with a node pool and the option to add a new tag.
Changes 🔄
Preview 📷
Screen.Recording.2024-12-05.at.12.46.32.PM.mov
How to test 🧪
Verification steps
(How to verify changes)
Add a Tag
button at the bottom of a Node Pool tableAuthor 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 ✅