-
Notifications
You must be signed in to change notification settings - Fork 163
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
Tooltip for disabled kebab is missing in Manage permissions page #3346
Tooltip for disabled kebab is missing in Manage permissions page #3346
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3346 +/- ##
==========================================
+ Coverage 84.81% 84.83% +0.01%
==========================================
Files 1321 1326 +5
Lines 29447 29678 +231
Branches 8038 8119 +81
==========================================
+ Hits 24976 25176 +200
- Misses 4471 4502 +31
... and 32 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
small nit otherwise lgtm
@@ -173,9 +178,18 @@ const RoleBindingPermissionsTableRow: React.FC<RoleBindingPermissionsTableRowPro | |||
/> | |||
</SplitItem> | |||
</Split> | |||
) : isDefaultGroup ? ( | |||
<Tooltip content="The default group always has access to model registry"> |
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.
full stop missing at the end according to design
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.
Good catch! Added.
@yih-wang can you have a look at the screenshot, please? |
@YuliaKrimerman can you add manual testing instructions in the "How has this been tested?" section. It will really be helpful for anyone outside of our scrum and/or QAing. For example, Instructions like 1. Go to Model registry settings table in the Admin section 2. Go to Manage Permissions 3. Check whether the first group in "Groups" section in the Users tab has a disabled kebab menu and a tooltip like the screenshot. |
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.
/lgtm
Tested whether the tooltip is present.
LGTM, thanks @YuliaKrimerman ! |
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.
LGTM. Shame that we can't just pass a disabledTooltip
into ActionsColumn, but that's a PF problem for another time and maybe not worth addressing.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: manaswinidas, mturley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
RHOAIENG-13050
Description
Added a "The default group always has access to model registry" Tooltip for disabled kebab in Manage permissions page when you hover over it
How Has This Been Tested?
On the UI go to Model registry settings table in the Admin section. Click on Manage Permissions Verify whether the first group in "Groups" section in the Users tab has a disabled kebab menu and a tooltip like the screenshot is showing up when we hover over the disabled kebab.
Test Impact
Fixed the tests to pass with the Button now
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main