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

fix: [M3-7407]: TableCell with ActionMenu incorrect size and border placement #9915

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Nov 17, 2023

Description 📝

Something about our TableCell with an Action Menu is not working well in some tables. The ActionMenu is causing the TableCell to be the wrong size and position the border incorrectly. ☹️

The Fix 🛠️

  • Removes the static height assigned to a TableCell with the actionCell prop

Preview 📷

Before After
before-zoomed after-zoomed
before after

How to test 🧪

Prerequisites

  • Turn on the MSW

Verification steps

  • Go to http://localhost:3000/loadbalancers/0/routes
  • Observe that the UI bug shown in the screenshot is resolved
  • Please help me verify this does not cause breaking changes elsewhere in the app

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

@bnussman-akamai bnussman-akamai added Bug Fixes for regressions or bugs UX/UI Changes for UI/UX to review labels Nov 17, 2023
@bnussman-akamai bnussman-akamai self-assigned this Nov 17, 2023
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner November 17, 2023 15:36
@bnussman-akamai bnussman-akamai requested review from cpathipa and cliu-akamai and removed request for a team November 17, 2023 15: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 fixing!

✅ Confirmed the table row bug is fixed in the AGLB routes page.

Spot checked a few tables throughout the app, checking at various screen sizes:
✅ Confirmed no regressions in kubernetes tables.
✅ Confirmed no regressions in linodes plan table.
✅ Confirmed no regressions in firewall rules table.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Nov 17, 2023
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

✅ confirmed that bug is gone from AGLB routes tab!

-- some other spot checks for table cells with actionCell prop passed in:
✅ confirmed VPC table and VPC subnets table still looks as expected
✅ confirmed volumes table still looks as expected
✅ confirmed domains table still looked as expected
✅ confirmed server transfers table still looked as expected
✅ confirmed NodeBalancers landing table still looked as expected
✅ Stackscripts table still looked as expected

Had a quick question that's not part of this PR, but is there a reason why we style the StackScript table header differently than the other table headers - specifically why it doesn't have the divider lines for each cell?
image
vs others:
image
image

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 20, 2023
@bnussman-akamai
Copy link
Member Author

@coliu-akamai

I don't have a good answer for you. I think it honestly might just be a regression or legacy thing. I can't think of a reason for it to not match all other tables.

@bnussman-akamai bnussman-akamai merged commit 2470131 into linode:develop Nov 20, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Bug Fixes for regressions or bugs UX/UI Changes for UI/UX to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants