Skip to content
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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Dec 4, 2024

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 🔄

  • Added Tags to Kubernetes Node Pool table

Preview 📷

Screen.Recording.2024-12-05.at.12.46.32.PM.mov

How to test 🧪

Verification steps

(How to verify changes)

  • Go to a Kubernetes cluster's details page
  • Test the Add a Tag button at the bottom of a Node Pool table
yarn cy:run -s "cypress/e2e/core/kubernetes/lke-update.spec.ts"
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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@hana-akamai hana-akamai added the LKE Related to Linode Kubernetes Engine offerings label Dec 4, 2024
@hana-akamai hana-akamai self-assigned this Dec 4, 2024
@hana-akamai hana-akamai marked this pull request as ready for review December 5, 2024 17:50
@hana-akamai hana-akamai requested review from a team as code owners December 5, 2024 17:50
@hana-akamai hana-akamai requested review from cliu-akamai, carrillo-erik, harsh-akamai and mjac0bs and removed request for a team December 5, 2024 17:50
Copy link

github-actions bot commented Dec 5, 2024

Coverage Report:
Base Coverage: 86.78%
Current Coverage: 86.78%

Copy link
Contributor

@mjac0bs mjac0bs left a 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.

packages/api-v4/.changeset/pr-11368-added-1733420616390.md Outdated Show resolved Hide resolved
Comment on lines 173 to 188
<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>
Copy link
Contributor

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?

Screenshot 2024-12-06 at 9 24 37 AM

Screenshot 2024-12-06 at 9 26 07 AM

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.

Copy link
Contributor Author

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

Copy link
Contributor

@mjac0bs mjac0bs Dec 11, 2024

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

@hana-akamai hana-akamai requested a review from mjac0bs December 11, 2024 16:53
Copy link
Contributor

@mjac0bs mjac0bs left a 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.

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Dec 11, 2024
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 466 passing tests on test run #12 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing466 Passing2 Skipped102m 11s

Copy link
Contributor

@harsh-akamai harsh-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I have added 2 tags that are pretty short but one gets overlapped by the icon(this is for desktop view). I think this has come up with the new updates as the preview video doesn't have this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add'tl Approval Needed Waiting on another approval! LKE Related to Linode Kubernetes Engine offerings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants