-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
fixed #380 console warning with tooltip #418
Conversation
tests/test_tooltip.py
Outdated
dash_duo.wait_for_text_to_equal("#text", "Text") | ||
dash_duo.find_element("#text").click() | ||
|
||
time.sleep(.2) # Adjust if needed based on any transition delays |
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.
Do you really need the explicit delay? Is the find_element
below not sufficient?
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.
hmm, it failed once and I thought it was the issue, but I changed other things as well. I'll give it another go without 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.
It passed when I ran it locally, but failed here. I'll put the delay back and see if it passes
@@ -15,7 +15,7 @@ interface Props extends TooltipBaseProps, DashBaseProps { | |||
const FloatingTooltip = (props: Props) => { | |||
const { children, boxWrapperProps, setProps, loading_state, ...others } = | |||
props; | |||
const boxProps = { w: "fit-content", ...boxWrapperProps }; | |||
const boxProps = { w: "fit-content", key: "tooltip-target", ...boxWrapperProps }; |
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.
Is there ever a case you could get two of these boxes as siblings? It's unclear to me why this is necessary at all given the nesting structure of the react components it generates, Box
is the only child of Tooltip.Floating
, so why would it need a key? This makes me speculate there may be something funny going on when Mantine renders this... and two siblings with the same key could be worse than no key at all. Two components with tooltips next to each other? Two tooltips on the same 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.
Yes, I'm concerned about this as well. Do you have any other suggestions? I did try an app with a few tooltips on a page and it worked fine. I'll try it with two tooltips on the same 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.
Actually, you can't have 2 tooltips on the same component because the Tootlip children
must be a single component:
dmc.Tooltip(
id="my-tooltip",
children=dmc.Button("some text", id="my-text"),
label="Tooltip text",
),
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.
How about using a random number as a key?
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.
OK - let's leave it then. In principle we could do a random number via useState
or some such so that it's retained between renders... but it doesn't seem like this complexity is necessary.
Co-authored-by: Alex Johnson <[email protected]>
I tried it with nested tooltips and it seemed to work
|
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.
💃
@alexcjohnson do you think this is really OK, or should we update to use a random number as a key? |
ok, mystery solved! |
closes #380
Fixed by adding a key to the
boxProps