Make Label Cursor Hoverable #3332
Conversation
There was a problem hiding this comment.
I think the label should only be hoverable for "Unavailable" and any other labels where we are showing a popover. There are no actions for "Available" and "Progressing" AFAIR - so no use making it hoverable(and confusing the users). Also, I think it's a bad practice to use void 0(makes it inaccessible for screen readers- reference link) Can we use a button instead? - I think button is hoverable by default.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3332 +/- ##
==========================================
+ Coverage 84.80% 85.08% +0.28%
==========================================
Files 1315 1326 +11
Lines 29491 29707 +216
Branches 8056 8132 +76
==========================================
+ Hits 25009 25277 +268
+ Misses 4482 4430 -52
... and 87 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@manaswinidas Made it hoverable only for the 2 needed statuses. Kept it as Label because of the color prop vs button. addressed the void 0 issue as well |
| <Label | ||
| {...(isClickable | ||
| ? { | ||
| onClick: () => { |
There was a problem hiding this comment.
not sure if we should prefer this over style={{ cursor: 'pointer' }} 🤔
(inspiration from #3344 - let me know if this works though.)
There was a problem hiding this comment.
It's already looks like the hand pointer, just like in the video. Or is it a different way we want it to look like?
There was a problem hiding this comment.
I tried to add that style but it's not changing anything from what I see - it remains the pointing hand ( like in the picture but just white and slightly bent)
There was a problem hiding this comment.
I think maybe the suggestion was to avoid the noop function, but I think it's probably fine either way - inline styles are also not great 🤷
@YuliaKrimerman can you just add more detail to the comment in that noop function, something like "Click event is handled by the Popover parent, this prop enables clickable styles in the PatternFly Label"?
| <Label | ||
| {...(isClickable | ||
| ? { | ||
| onClick: () => { |
There was a problem hiding this comment.
I think maybe the suggestion was to avoid the noop function, but I think it's probably fine either way - inline styles are also not great 🤷
@YuliaKrimerman can you just add more detail to the comment in that noop function, something like "Click event is handled by the Popover parent, this prop enables clickable styles in the PatternFly Label"?
| @@ -118,8 +117,24 @@ export const ModelRegistryTableRowStatus: React.FC<ModelRegistryTableRowStatusPr | |||
| } | |||
| } | |||
|
|
|||
| const isClickable = | |||
There was a problem hiding this comment.
I think you can have this logic based on whether there's a popover instead of based on status.
const isClickable = popoverTitle && popoverMessages.length;
|
@manaswinidas regarding the Also I think using just the cursor:pointer style wouldn't be good enough because the |
There was a problem hiding this comment.
LGTM - @manaswinidas if you disagree with the approval let's follow up
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
mturley
dismissed
manaswinidas’s stale review
Comments addressed - may require followup conversation but shouldn't block the PR
https://issues.redhat.com/browse/RHOAIENG-13047
Description
Made the labels indicate they are hoverable in Settings page
Screen.Recording.2024-10-14.at.3.47.21.PM.mov
How Has This Been Tested?
On the UI
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main