Position tooltip on value change

[CumpsD]

The tooltip was not centered anymore after a value change (for example, localisation change). Triggering it again fixes it.

[oliviertassinari]

trailing spaces

[hai-cea]

Thanks @CumpsD - I think I might have misspoken on gitter because this may have to be done in componentDidUpdate. Is this working for you?

[CumpsD]

This works. If I did it in componentDidUpdate, I got a stack overflow,
getting into a loop.
On 21 Jul 2015 12:30 am, "Hai Nguyen" [email protected] wrote:

In src/tooltip.jsx
#1205 (comment):

@@ -26,6 +26,10 @@ let Tooltip = React.createClass({
this._setTooltipPosition();
},

• componentWillReceiveProps() {

Thanks @CumpsD https://github.com/CumpsD - I think I might have
misspoken on gitter because this may have to be done in componentDidUpdate.
Is this working for you?

—
Reply to this email directly or view it on GitHub
https://github.com/callemall/material-ui/pull/1205/files#r35051913.

[cgestes]

_setTooltipPosition() should be protected against calling setState if the value did not change.

Then I guess it will works in componentDidUpdate.

But because it does a setState, it means render will always cause another render when offsetWidth change. (which is bad)

Maybe offsetWidth should not be a state, and the offset should always be calculated and set in componentDidUpdate.

[cgestes]

would the css 'calc' function works in that case?

[CumpsD]

I'll leave that to @hai-cea and @oliviertassinari since I don't know enough about it :/

[oliviertassinari]

Yeah, the current implementation can be improved. The component renders two times when he is mounted. This is bad.
The solution is to remove offsetWidth from the state. Then either use css tricks to make the tooltip centered or use vanilla js.

[CumpsD]

Is that something you can do?

[hai-cea]

@oliviertassinari @cgestes Is there any harm in merging this and address the implementation improvement in a separate PR?

[cgestes]

is this commit correct? I think it calculates the offsetWidth of the previous render, not of the current one.

[CumpsD]

@cgestes I thought it was wrong to when I did it at first, but it actually works.

[CumpsD]

A visual explanation:

Start:

Tooltip updated:

Using the fix from this PR:

Start:

Tooltip updated:

[hai-cea]

Looks good to me.

[hai-cea]

Thanks @CumpsD !