Skip to content
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

show tooltips for func overlay without transition #5631

Merged
merged 2 commits into from Feb 1, 2021

Conversation

mmomtchev
Copy link
Contributor

#5621

How about this? The user will have to add the classes himself when he is using a function overlay

@kyletsang
Copy link
Member

I think this will need to target the bs5 branch since it's breaking behavior.

@jquense
Copy link
Member

jquense commented Jan 30, 2021

I'm tempted to call this a bug fix...do you think anyone would be depending on the tooltip not working without a transition? I guess someone might be using it to toggle visibility?

@kyletsang
Copy link
Member

I was initially thinking people were using the show prop that's passed along to toggle visibility manually, but maybe a lot of people just use transitions by default. This sounds pretty low risk, so I'm down for merging this.

@jquense
Copy link
Member

jquense commented Jan 30, 2021

We can always revert if it causes a problem

src/Overlay.tsx Outdated
@@ -205,6 +205,7 @@ function Overlay({
...overlayProps,
placement,
show,
className: !transition && show && 'show',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
className: !transition && show && 'show',
...(!transition && show && { className: 'show' }),

This change will avoid overwriting className when transition is true and spreading props after a className is declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overlayProps won't contain the className if overlay is a function? The className will probably be hard-coded in the function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no className in the overlayProps. This is what's passed back from react-overlays:

https://github.com/react-bootstrap/react-overlays/blob/f9f1a14752a8d8ff9aa7c7d207aa0c413ca70660/src/Overlay.tsx#L117-L130

The above suggestion will prevent the below className="abc" from being overwritten when transition=true.

(props) => <Tooltip className="abc" {...props}>my tooltip</Tooltip

@kyletsang kyletsang linked an issue Jan 30, 2021 that may be closed by this pull request
@mmomtchev
Copy link
Contributor Author

@jquense Not that many in fact 😄
The main problem is of course that recent versions of create-react-app have strict mode enabled - but no matter one's goodwill - and I tried hard - you end up disabling it for any serious project

@kyletsang kyletsang merged commit 278eff7 into react-bootstrap:master Feb 1, 2021
@kyletsang
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transition={false} prevents Tooltip from showing
3 participants