-
Notifications
You must be signed in to change notification settings - Fork 39
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
🐛 Add tooltip for weight column #1433
Conversation
ibolton336
commented
Oct 4, 2023
- Adds tooltip to weight answer table column https://issues.redhat.com/browse/MTA-1351?filter=-1
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1433 +/- ##
=======================================
Coverage 41.07% 41.07%
=======================================
Files 139 139
Lines 4404 4404
Branches 1010 1010
=======================================
Hits 1809 1809
Misses 2583 2583
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View 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.
I recommend the style change, but it isn't a blocker.
{...getThProps({ columnKey: "weight" })} | ||
info={{ | ||
tooltip: | ||
"This column shows the associated risk weight of an answered question. A red 'X' icon indicates High risk, a yellow warning icon indicates Medium risk, and a green checkmark indicates Low risk.", |
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.
Since the tooltip
prop can be a ReactNode
, I'd rather see a better styled tooltip:
tooltip: (
<TextContent>
<Text>This column shows the associated risk weight of an answered question.</Text>
<List isPlain>
<ListItem>{getIconByRisk("red")} is High risk</ListItem>
<ListItem>{getIconByRisk("yellow")} is Medium risk</ListItem>
<ListItem>{getIconByRisk("green")} is Low risk</ListItem>
</List>
</TextContent>
),
Doing some i18n and adding the messages to translation.json
would be nice.
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.
this looks great |
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.
Given @JustinXHale 's approval, this is ok with me. I still recommend the tooltip content change, but it is not a blocker.
Signed-off-by: ibolton336 <[email protected]>