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

change: [M3-8346] – UI modications for LKE Details summary panel & Node Pool tables #10685

Merged

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Jul 16, 2024

Description πŸ“

Some UI improvements for the LKE Details summary panel and Node Pool tables

Note

There is an existing issue noticeable in dark mode for LinodeEntityDetailBody where the border above the VPC section in the Linode Detail header extends past the left bounds of the table. I created M3-8354 to track this.

Changes πŸ”„

  • Add a header to the LKE Details summary panel containing "Summary" and the Kubernetes Dashboard and Delete Cluster buttons
    • Remove secondary button styling from buttons
  • Remove the <Paper /> behind the entire Node Pools section, replaced with a paper behind each Node Pool table
  • Place each Node Pool's plan and action buttons in a header, adjusting the styling of the action buttons to text instead of secondary buttons

Target release date πŸ—“οΈ

7/22/24

Preview πŸ“·

Before After
Screenshot 2024-07-17 at 10 31 10β€―AM Screenshot 2024-07-17 at 10 30 18β€―AM
Screenshot 2024-07-17 at 10 32 30β€―AM Screenshot 2024-07-17 at 10 32 30β€―AM

How to test πŸ§ͺ

Verification steps

Verify the changes described above for the LKE Details summary panel and the individual Node Pools tables.

Additionally, confirm there are no adverse impacts to the Linode Details header, since this PR made some modifications to EntityDetail.tsx which is used in LinodeEntityDetail.tsx.

As an Author I have 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

@dwiley-akamai dwiley-akamai added the Linode Disk Encryption (LDE) Relating to LDE project label Jul 16, 2024
@dwiley-akamai dwiley-akamai self-assigned this Jul 16, 2024
@dwiley-akamai dwiley-akamai changed the title change: [M3-8345] – UI modications for LKE Details summary panel & Node Pool tables change: [M3-8346] – UI modications for LKE Details summary panel & Node Pool tables Jul 17, 2024
header: JSX.Element;
noBodyBottomBorder?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced this prop to prevent this situation:

Screenshot 2024-07-16 at 11 36 53β€―AM

Without a footer, there would otherwise be a bottom border for the body.

With further improvements forthcoming as part of LKE-E, this may become unnecessary as the rough draft wireframes have tags moving into a footer section.

/>
</Grid>
<Stack sx={{ marginBottom: theme.spacing(3) }}>
<EntityDetail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shift to using EntityDetail here should be understood in the context of https://github.com/linode/manager/pull/10685/files#r1681361959. It's laying the foundation for further forthcoming LKE-E improvements

Comment on lines +36 to +40
{footer !== undefined && (
<GridFooter body={body} xs={12}>
{footer}
</GridFooter>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

another option is to leave footer as required and pass a <React.Fragment /> for it in KubeSummaryPanel.tsx, which is simpler but I would prefer not using fragments in an unintended fashion like that.

@dwiley-akamai dwiley-akamai marked this pull request as ready for review July 17, 2024 16:58
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner July 17, 2024 16:58
@dwiley-akamai dwiley-akamai requested review from jdamore-linode, bnussman-akamai and carrillo-erik and removed request for a team July 17, 2024 16:58
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Hover color of the node row is too dark

Screenshot 2024-07-17 at 1 16 48β€―PM

Mobile spacing is a bit off

Screenshot 2024-07-17 at 1 16 11β€―PM

LinkButtons used where I'd expect buttons Can we make the Dashboard and Delete more like the other buttons on this page? Having them as LinkButtons feels odd because both Linodes and VPCs use buttons

Screenshot 2024-07-17 at 1 15 46β€―PM

All of that feedback is non-blocking because this page has already been somewhat broken and this seems to be slightly better βœ…

@dwiley-akamai
Copy link
Contributor Author

Hover color of the node row is too dark
Mobile spacing is a bit off
LinkButtons used where I'd expect buttons
All of that feedback is non-blocking because this page has already been somewhat broken and this seems to be slightly better βœ…

Thanks for catching the buttons, fixed that in the latest commit.

For the mobile spacing, we have M3-8348 which will be pulled into the LKE-E effort. The node row hover color is like that in prod as well but I agree that it's too dark and should match other similar rows

Copy link

Coverage Report: βœ…
Base Coverage: 82.35%
Current Coverage: 82.36%

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 for the improvements and prep work (+ TODO) for LKE-E changes to come. I noticed no regressions to the Linode entity details. LKE details looks good, aside from the issues that were already raised to be tackled in M3-8348. 🚒

@dwiley-akamai dwiley-akamai merged commit 669f54d into linode:develop Jul 17, 2024
19 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-8346-lke-details-ui-refinements branch July 17, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linode Disk Encryption (LDE) Relating to LDE project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants