-
Notifications
You must be signed in to change notification settings - Fork 0
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
75 coreui tooltip component #932
Conversation
Almost ready for undrafting. The only thing I can't yet figure out is why the MesaTooltips in the Mesa header cells won't show. Any thoughts? |
…athDB/web-monorepo into 75-coreui-tooltip-component
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 started to review this. See my preliminary comments about WDKClientTooltip
below
packages/libs/wdk-client/src/Components/AttributeFilter/Histogram.jsx
Outdated
Show resolved
Hide resolved
packages/libs/wdk-client/src/Components/AttributeFilter/FieldList.jsx
Outdated
Show resolved
Hide resolved
@@ -56,7 +53,7 @@ export default function UserDatasetStatus(props: Props) { | |||
: 'exclamation-circle'; | |||
const children = <Icon className="StatusIcon" fa={faIcon} />; | |||
const visibleContent = props.useTooltip ? ( | |||
<Tooltip content={content}>{children}</Tooltip> | |||
<Tooltip title={content}>{children}</Tooltip> |
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 tooltip is still not firing off in the Status
column of the My Data Sets
table. I think this component just needs refactoring. I think if we render the Tooltip above the Link component, it'll work just fine. If you don't mind, could you open an issue for this and I'll do the follow up work in the VDI client branch after merging this work into it? It'll create a big enough merge conflict in that component such that it makes just as much sense for me to handle it!
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.
Here's the new ticket! VEuPathDB/EdaNewIssues#776
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.
Just a few more notes, some of which can be follow-up work. This is a big enough overhaul that we may also want to tip off some stakeholders to be on the lookout for any broken tooltips, similar to my warning re: the jquery DataTable retirement.
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.
All other testing looks good to me, so I'm going to go ahead and approve so that after you address the falsy title issue you can go ahead and merge without further delay!
Fixes VEuPathDB/CoreUI#75