-
Notifications
You must be signed in to change notification settings - Fork 379
feat: [UIE-8731] - Roles Table and Users Table ui fix #12233
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
base: develop
Are you sure you want to change the base?
feat: [UIE-8731] - Roles Table and Users Table ui fix #12233
Conversation
8b72506
to
531b6b2
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 @mpolotsk-akamai, changes looked good overall, but the unit test failure does seem related. Seems like maybe updating tests to mock the intersection observer is necessary? Will approve once that's addressed.
Here are a couple other things I noticed for consideration:
- The tooltip icon for Permissions seems oddly placed. I would expect it to be on the same line as Permissions, to its right. Would you expect the same? This is an existing issue in dev, and maybe it already has an intended ticket for follow up.
- A couple issue I noticed on small mobile screens:
- The role name is illegible due to wrapping.
- The Assign Role button extends beyond the table.
- I am wondering about mobile view in general for this table. Did UX give any insight on columns that could be dropped/how the table could be condensed to be more mobile-friendly? (Description takes up a lot of space, but seems important, for ex.) That's a larger question, and I know not necessarily in scope for this PR.
Screen.Recording.2025-05-16.at.9.08.41.AM.mov
@mjac0bs ,
|
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 addressing feedback!
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.
Can we pull out the changes to useCalculateHiddenItems and handle those separately?
|
||
import type { PermissionType } from '@linode/api-v4'; | ||
|
||
/** | ||
* Custom hook to calculate hidden items | ||
*/ | ||
export const useCalculateHiddenItems = ( |
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.
What was the issue with the previous implementation? A lot of time was spent to avoid unnecessary calculations, if we are reverting that, we need to document why that is necessary. There are no screenshots showing this change, and no unit tests to ensure we aren't breaking something.
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 the review, @corya-akamai !
The previous implementation had a few issues:
- The expand button in role rows was showing the total number of assigned permissions, not just the hidden ones.
- It was showing “+N” even when all items were already visible (e.g., only 3 permissions).
- The hidden permissions count only updated after expanding and collapsing. If there were no hidden items, the “Expand (+N)” button would disappear completely after collapsing again.
From my understanding, the core issue was that getBoundingClientRect() was called before the layout had fully settled. Combined with -webkit-line-clamp, which visually hides overflow content without actually laying it out, this led to inaccurate measurements, causing the logic to assume all items were hidden, even when they were visible.
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.
I’ve also added screenshots that show the issues.
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.
@mpolotsk-akamai awesome thanks!
…luding button styling, layout, and permissions toggle
a2ec668
to
6e4d382
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.
Can we pull that useCalculateHidden into another PR?
After discussion with @corya-akamai and @aaleksee-akamai , I’ve removed the fix for the Expand/Hide functionality from this PR. That issue will be addressed separately in a dedicated PR. |
Cloud Manager UI test results🎉 605 passing tests on test run #9 ↗︎
|
Description 📝
Roles Table and Users Table UI fix
Changes 🔄
List any change(s) relevant to the reviewer.
Target release date 🗓️
June 3rd
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
Go to the Users tab
Go to the Roles tab
Go to the Permissions section
firewall_creator
roleAuthor 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 ✅