[SpeedDial] Allow tooltip to always be displayed #12590
Conversation
| this.setState({ tooltipOpen: true }); | ||
| }; | ||
|
|
||
| componentDidUpdate = prevProps => { | ||
| if (!this.props.tooltipAlwaysOpen) return; | ||
| if (prevProps.open === this.props.open) return; |
There was a problem hiding this comment.
You could perhaps combine these two line if (!a || b === c) return;
| if (!this.props.open && this.state.tooltipOpen) { | ||
| this.setState({ tooltipOpen: false }); | ||
| } else if (!this.state.tooltipOpen) { | ||
| setTimeout(() => this.setState({ tooltipOpen: true }), this.props.delay + 100); |
There was a problem hiding this comment.
You'll need to clear the timeout in componentWillUnmount
| /** | ||
| * Option to always display the tooltip (usually applicable on mobile) | ||
| */ | ||
| tooltipAlwaysOpen: PropTypes.bool, |
There was a problem hiding this comment.
Lets go with tooltipOpen for brevity.
|
@mbrookes I updated the PR with the changes you requested. |
|
@mbrookes good to go? |
|
|
||
| The SpeedDialActions tooltips can be be displayed persistently so that users don't have to long-press in order to see the tooltip on touch devices. | ||
|
|
||
| It is enabled here across all devices for demo purposes, but in production could use the isTouch logic to conditionally set the property. |
There was a problem hiding this comment.
- but in production could use the isTouch logic
+ but in production it could use the `isTouch` logicNot sure about the typo but the formatting change would be nice.
There was a problem hiding this comment.
Not really a typo, but yours is grammatically clearer. Agree on the formatting.
|
Have you considered using |
|
@eps1lon |
| this.setState({ tooltipOpen: false }); | ||
| }; | ||
|
|
||
| handleTooltipOpen = () => { | ||
| if (this.props.tooltipOpen) return; | ||
| this.setState({ tooltipOpen: true }); |
There was a problem hiding this comment.
@hashwin I was just looking at this and this seems brittle since other methods can still change the state of tooltipOpen. Not sure if we can do anything about this though.
There was a problem hiding this comment.
I see your point, but yeah I don't really see another way of handling this without the state and state itself can be changed from anywhere in the component. In fact, we need to manually set the state here for this feature to work: https://github.com/mui-org/material-ui/pull/12590/files#diff-3c8c197bf9114d5f5a42f05daae04497R59
One thing we can maybe do here is to change the method name from handleTooltipOpen to something like onHover so that it explicitly says that nothing should be done on hover if the prop tooltipOpen is given. Thoughts?
| componentDidUpdate = prevProps => { | ||
| if (!this.props.tooltipOpen || prevProps.open === this.props.open) return; | ||
| if (!this.props.open && this.state.tooltipOpen) { | ||
| this.setState({ tooltipOpen: false }); |
There was a problem hiding this comment.
Do you see a way to move this to getDerivedStateFromProps and save a rendercycle?
| state.tooltipOpen = false; | ||
| } | ||
| return state; | ||
| }; |
There was a problem hiding this comment.
That is not how getDerivedStateFromProps works.
Return null for no change or an object that should be merged.
| } | ||
| }; | ||
|
|
||
| componentWillUnmount = () => this.timeout && clearTimeout(this.timeout); |
There was a problem hiding this comment.
Just call clearTimeout. It won't complain about non-ids but will cause issues in browsers that actually use 0 as ids.
|
@hashwin Nice first contribution! 👍 |
mbrookes
added
the
component: speed dial
This is an attempt to rectify the issues that were present in #11103
The logic here is that since the positioning of the tooltip was incorrect due to the rendering time of the SpeedDialAction that comes in via a transition (plus an optional delay given by the
delayprop).This POC shows that the tooltips are indeed rendered in the correct position if there is a timeout given.
@mbrookes please take a look as you reviewed the previous PR. Thanks.