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

Rerender at initial position #388

Closed
an4ger opened this issue May 31, 2018 · 8 comments · May be fixed by Omrisnyk/npm-lockfiles#144
Closed

Rerender at initial position #388

an4ger opened this issue May 31, 2018 · 8 comments · May be fixed by Omrisnyk/npm-lockfiles#144

Comments

@an4ger
Copy link
Contributor

an4ger commented May 31, 2018

  1. Create tooltip with delayShow={300} delayHide={300} place="top" options.
  2. Move cursor to element with tooltip and wait for tooltip, it should appear not at the top position.
  3. Quick move cursor from element with tooltip and back.

Result: tooltip try to render with bottom arrow as at top place (initial option) and after that appear at right position. So, we have some blinking effect.
If set delay to 5 seconds we have the same effect.

https://codesandbox.io/s/j70q5w7p1w
tooltip

@an4ger an4ger changed the title Rerender on initial position Rerender at initial position May 31, 2018
@smakosh
Copy link

smakosh commented Jun 1, 2018

I'm having the same issue when the tooltip should appear near the corner of the window

@an4ger
Copy link
Contributor Author

an4ger commented Jun 5, 2018

Here some info after investigation:

mouseenter event trigger this function:

https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L256

and than we call

https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L404

here we have some problem:

 if (result.isNewState) {
      // Switch to reverse placement
      return this.setState(result.newState, () => {
        this.updatePosition()
      })
    }

in my case function updatePosition triggers 2 times:

  • first time result.isNewState = true, because in showTooltip function we set desiredPlace in state every time on mouseenter and that desiredPlace not equal current available position;
  • second time result.isNewState = false and all good;

So, it will be better to ignore first call of updatePosition function.

One of solution is to do some caching and try to set desiredPlace not from attribute but from last available place:
https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L286

lets try to change it this way:

this.state.place || desiredPlace: e.currentTarget.getAttribute('data-place') || this.props.place || 'top',

Is it good idea? Any suggestions?

@P0lip
Copy link
Contributor

P0lip commented Jun 5, 2018

Well, I didn't check it myself, yet I suppose you are right and updatePosition is called more than once.

The trick with desiredPlace may be a bit troublesome and may introduce new bugs, as it doesn't take into account any external changes that could affect the position of tooltip in the meantime.
By external changes, I mean window resize, page scroll or any other layout changes.
In your particular case, the item is always bound to the left top corner and always has a fixed position, so this would do the job, but in some other scenarios, it's not the case, unfortunately.

Examples:
I have a tooltip I want to display above the content. It's displayed below, because there is no enough space at the top of viewport. Once I scroll down, I have enough space and I can display it above.

I have a tooltip I want to display below the content. It's displayed above, because there is no enough space at the bottom of viewport. Once I scroll down, it should be displayed below. Instead, it's still displayed above and I can't see it fully as it's still stuck to the top of my screen, above the content.

Once again, I didn't test the change myself, so I may be totally wrong and the proposed solution may work like a charm.

I'd say that getPosition function should be refactored, as this is the pure source of error.
It should be produced the same results, since nothing on screen has changed, so the position of tooltip should be exactly the same for both calls.

@gris-martin
Copy link
Contributor

The way getPosition works in regards to desiredPlace is altered slightly in #391 (if it gets merged). Not sure if it relates directly to this issue, but you should take a look and see if it makes sense.

@gris-martin
Copy link
Contributor

gris-martin commented Jun 7, 2018

I did some more research, and this issue doesn't have to do with updatePosition being called twice, but rather with setState being called (and thus the component rendered) before we know which orientation the tooltip should have:
https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L283-L287
After the rendering, the orientation is then immediately updated to the correct one (and the component re-rendered) by a callback added in the same setState call. This callback calls the updateTooltip function, which in turn calls the updatePosition function.
https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L304-L306

While the updatePosition function is called twice (through recursion), setState is actually called at most one time (unless something goes wrong) during the callback.
https://github.com/wwayne/react-tooltip/blob/c44cc2d0aa89b7e6268a61a277326be6903b00ae/src/index.js#L404-L418

I agree that updatePosition and getPosition should probably be reworked, but it's not the cause of this issue (unless I'm completely mistaken!).

EDIT: I did a quick fix in my fork (https://github.com/hassanbot/react-tooltip/commit/9b2c115f716d46289c0cb7655987870f5ae5e6ac), but it feels a little hacky so maybe someone has a better suggestion?

@aronhelser
Copy link
Collaborator

I feel like your solution is reasonable, and it seems like we don't have any other input. Should we go with it?

@an4ger
Copy link
Contributor Author

an4ger commented Jun 25, 2018

@hassanbot can you make pull request with this fix?

an4ger added a commit to an4ger/react-tooltip that referenced this issue Aug 20, 2018
getPosition has to be called before first setting the state (and thus rendering the tooltip) when
mouse enters, to take into account cases when the position is close to the client window edge

ReactTooltip#388
@diachini
Copy link

@aronhelser I believe this issue (#388) can be closed (#413 just didn't have "close"/"closes" in it to do so automatically).

@an4ger an4ger closed this as completed Sep 12, 2018
@an4ger an4ger reopened this Sep 12, 2018
@an4ger an4ger closed this as completed Sep 12, 2018
jafin pushed a commit to jafin/react-tooltip that referenced this issue Sep 30, 2022
# 1.0.0-alpha.1 (2022-04-03)

### Bug Fixes

* add aria hidden attribute to style tag ([ReactTooltip#703](https://github.com/CodeForked/react-tooltip/issues/703)) ([d60c2b7](CodeForked/react-tooltip@d60c2b7))
* **aftershow:** call afterShow only after state has fully updated ([54752e8](CodeForked/react-tooltip@54752e8))
* **aphrodite_jss_deprecation:** aphrodite_jss replaced with custom solution ([fcdf7f1](CodeForked/react-tooltip@fcdf7f1))
* **aphrodite_jss_deprecation:** aphrodite_jss replaced with custom solution ([92fcf5b](CodeForked/react-tooltip@92fcf5b))
* **build:** removing single quotes on cpy for windows shell ([ReactTooltip#632](https://github.com/CodeForked/react-tooltip/issues/632)) ([9c280af](CodeForked/react-tooltip@9c280af))
* **colors:** allow customizable text, background, border, arrow colors ([9a85253](CodeForked/react-tooltip@9a85253))
* **compability:** add polyfill and change styles ([ReactTooltip#706](https://github.com/CodeForked/react-tooltip/issues/706)) ([b6e9a1c](CodeForked/react-tooltip@b6e9a1c))
* deleting warning in peer dependencies ([f30ae74](CodeForked/react-tooltip@f30ae74))
* do not delay show if tooltip is already shown ([ReactTooltip#676](https://github.com/CodeForked/react-tooltip/issues/676)) ([e8b9d84](CodeForked/react-tooltip@e8b9d84))
* **domexception:** revert previous changed for unexpected behavior ([85e38bb](CodeForked/react-tooltip@85e38bb)), closes [ReactTooltip#667](https://github.com/CodeForked/react-tooltip/issues/667)
* effect and type not properly applied at first render ([a8d0e51](CodeForked/react-tooltip@a8d0e51))
* **event:** expose the original event to `afterShow` and `afterHide` ([e2f973e](CodeForked/react-tooltip@e2f973e))
* **example:** 'made dev' works again, small fixes. ([7b286bb](CodeForked/react-tooltip@7b286bb)), closes [ReactTooltip#328](https://github.com/CodeForked/react-tooltip/issues/328) [ReactTooltip#341](https://github.com/CodeForked/react-tooltip/issues/341)
* **example:** <p> warning from react, make text match code. ([7c4c979](CodeForked/react-tooltip@7c4c979))
* **examples:** add SVG example ([72a98d7](CodeForked/react-tooltip@72a98d7))
* fix ie edge CustomEvent bug ([ReactTooltip#567](https://github.com/CodeForked/react-tooltip/issues/567)) ([b7f04f7](CodeForked/react-tooltip@b7f04f7)), closes [ReactTooltip#498](https://github.com/CodeForked/react-tooltip/issues/498)
* **getPosition Util:** Remove shouldUpdatePlace check from position check ([1f8a054](CodeForked/react-tooltip@1f8a054)), closes [ReactTooltip#574](https://github.com/CodeForked/react-tooltip/issues/574)
* **getposition:** properly determine parents with will-change: transform ([3a76250](CodeForked/react-tooltip@3a76250))
* **getPosition:** updated getPosition to fix 'maximum update depth' ([8fda305](CodeForked/react-tooltip@8fda305))
* **githubPage:** updating github page build in travis ([87b810a](CodeForked/react-tooltip@87b810a))
* **html:** remove sanitize-html-react, reduce package size ([177ac11](CodeForked/react-tooltip@177ac11)), closes [ReactTooltip#429](https://github.com/CodeForked/react-tooltip/issues/429)
* **index.js:** add missing argument so tooltip hides. ([4d3661b](CodeForked/react-tooltip@4d3661b))
* **index.js:** fix exception when testing with Jest ([ReactTooltip#682](https://github.com/CodeForked/react-tooltip/issues/682)) ([f885f1f](CodeForked/react-tooltip@f885f1f))
* **index.js:** fix state initialization ([69dea07](CodeForked/react-tooltip@69dea07))
* **index.js:** Replaced the deprecated `componentWillReceiveProps`. ([80b71ed](CodeForked/react-tooltip@80b71ed))
* **index.js:** Use correct orientation when mouse enters ([4a0da8b](CodeForked/react-tooltip@4a0da8b)), closes [ReactTooltip#388](https://github.com/CodeForked/react-tooltip/issues/388)
* install dependencies in example travis ([7ba8b28](CodeForked/react-tooltip@7ba8b28))
* **isCapture:** better guard that preserves logic ([28b8493](CodeForked/react-tooltip@28b8493))
* **isCapture:** guard use of currentTarget ([4f89a2d](CodeForked/react-tooltip@4f89a2d))
* **lint:** styles are now linted with stylelint ([3c17198](CodeForked/react-tooltip@3c17198))
* made it possible to pass uuid instead of generating one internally ([ReactTooltip#583](https://github.com/CodeForked/react-tooltip/issues/583)) ([083edfb](CodeForked/react-tooltip@083edfb)), closes [ReactTooltip#580](https://github.com/CodeForked/react-tooltip/issues/580)
* mark prop-types and uuid as external to avoid bundling them ([ReactTooltip#582](https://github.com/CodeForked/react-tooltip/issues/582)) ([fb60855](CodeForked/react-tooltip@fb60855))
* modifying example ([9dc0b2e](CodeForked/react-tooltip@9dc0b2e))
* **no_var:** no vars allowed ([c591804](CodeForked/react-tooltip@c591804))
* **overridePosition:** providing currentEvent in overridePosition ([ReactTooltip#563](https://github.com/CodeForked/react-tooltip/issues/563)) ([3e75a09](CodeForked/react-tooltip@3e75a09)), closes [ReactTooltip#513](https://github.com/CodeForked/react-tooltip/issues/513)
* performance issue caused by excessive use of clearTimeout/Interval ([22aea50](CodeForked/react-tooltip@22aea50))
* providing currentTarget in overridePosition ([ReactTooltip#564](https://github.com/CodeForked/react-tooltip/issues/564)) ([22c3bac](CodeForked/react-tooltip@22c3bac))
* **pr:** package.json fix; refactoring to exclude dependencies ([fdc17d4](CodeForked/react-tooltip@fdc17d4))
* release event listners ([ReactTooltip#534](https://github.com/CodeForked/react-tooltip/issues/534)) ([7cc1203](CodeForked/react-tooltip@7cc1203))
* **selector:** Add support for shadow DOM elements ([99be4d1](CodeForked/react-tooltip@99be4d1))
* **selector:** lint fixes ([873c2a8](CodeForked/react-tooltip@873c2a8))
* set aria-describedby value wrong when custom id ([a04d26c](CodeForked/react-tooltip@a04d26c))
* **showtooltip:** check if tooltipRef is undefined ([ReactTooltip#623](https://github.com/CodeForked/react-tooltip/issues/623)) ([f63eab2](CodeForked/react-tooltip@f63eab2))
* skip warning in example ([a555060](CodeForked/react-tooltip@a555060))
* **src/index.js:** add accessibility support for tabbing ([ReactTooltip#695](https://github.com/CodeForked/react-tooltip/issues/695)) ([ae936a5](CodeForked/react-tooltip@ae936a5))
* **src/index.js:** hide tooltip if blurred (tabbed out) ([ReactTooltip#699](https://github.com/CodeForked/react-tooltip/issues/699)) ([e0a2a1d](CodeForked/react-tooltip@e0a2a1d))
* **src/index.js:** Overwrite `delayHide` on scroll ([7a2d0b3](CodeForked/react-tooltip@7a2d0b3)), closes [ReactTooltip#474](https://github.com/CodeForked/react-tooltip/issues/474)
* **staticMethods:** fixing IE event bug ([ReactTooltip#569](https://github.com/CodeForked/react-tooltip/issues/569)) ([9acc591](CodeForked/react-tooltip@9acc591))
* string into example ([356821b](CodeForked/react-tooltip@356821b))
* **style injection:** change style injection default root ([a00c5b7](CodeForked/react-tooltip@a00c5b7)), closes [ReactTooltip#665](https://github.com/CodeForked/react-tooltip/issues/665)
* **styles:** add styles for shadow dom ([00d1539](CodeForked/react-tooltip@00d1539)), closes [ReactTooltip#597](https://github.com/CodeForked/react-tooltip/issues/597)
* **styles:** change style injection way ([ReactTooltip#668](https://github.com/CodeForked/react-tooltip/issues/668)) ([1e10cce](CodeForked/react-tooltip@1e10cce)), closes [ReactTooltip#650](https://github.com/CodeForked/react-tooltip/issues/650)
* **tooltip:** sanitize HTML to prevent XSS ([182df11](CodeForked/react-tooltip@182df11))
* **type:** added role property to types ([ReactTooltip#679](https://github.com/CodeForked/react-tooltip/issues/679)) ([9b49395](CodeForked/react-tooltip@9b49395))
* **type:** Fix global method parameter type ([ReactTooltip#585](https://github.com/CodeForked/react-tooltip/issues/585)) ([5e2b8db](CodeForked/react-tooltip@5e2b8db))
* **types:** adding types filename to package ([ReactTooltip#579](https://github.com/CodeForked/react-tooltip/issues/579)) ([05d8de2](CodeForked/react-tooltip@05d8de2))
* **types:** adding typescript d.ts file into dist ([e6300f7](CodeForked/react-tooltip@e6300f7)), closes [ReactTooltip#579](https://github.com/CodeForked/react-tooltip/issues/579)
* update uuid module ([d937e59](CodeForked/react-tooltip@d937e59))
* updating styles using transferSas ([f2f7804](CodeForked/react-tooltip@f2f7804))
* using css into example ([7d343af](CodeForked/react-tooltip@7d343af))
* using sass styles with rollup ([bb6fe48](CodeForked/react-tooltip@bb6fe48))
* **uuid:** Use uuid package for unique class names ([ReactTooltip#566](https://github.com/CodeForked/react-tooltip/issues/566)) ([c2c2243](CodeForked/react-tooltip@c2c2243))
* validate lint in pretest ([ad7add0](CodeForked/react-tooltip@ad7add0))

* Merge pull request ReactTooltip#550 from wwayne/refactoring ([4609833](CodeForked/react-tooltip@4609833)), closes [ReactTooltip#550](https://github.com/CodeForked/react-tooltip/issues/550)

### Features

* adding typescript type defs ([ReactTooltip#571](https://github.com/CodeForked/react-tooltip/issues/571)) ([cb2b921](CodeForked/react-tooltip@cb2b921))
* **clickable-prop:** add clickable prop ([a75b2be](CodeForked/react-tooltip@a75b2be)), closes [ReactTooltip#417](https://github.com/CodeForked/react-tooltip/issues/417)
* **component:** adding "padding" property to customize padding style ([9ae765a](CodeForked/react-tooltip@9ae765a))
* convert to typescript ([dc547c1](CodeForked/react-tooltip@dc547c1))
* getContent(dataTip) ([8bfbfc9](CodeForked/react-tooltip@8bfbfc9))
* **getContent:** update Travis, trigger semantic-release ([9617267](CodeForked/react-tooltip@9617267))
* **overridePosition:** Add "overridePosition" property to handle border cases and customize position ([ccb8b58](CodeForked/react-tooltip@ccb8b58))
* **overridePosition:** Added example ([7df8454](CodeForked/react-tooltip@7df8454))
* Small bug fix to previous commit ([19a8a67](CodeForked/react-tooltip@19a8a67))
* The only way to support styling react-tooltips with a strict csp is to inject the style.css di ([cf105a1](CodeForked/react-tooltip@cf105a1)), closes [ReactTooltip#340](https://github.com/CodeForked/react-tooltip/issues/340)
* **tooltip:** Add ability to hover on tooltip. Provide optional delay of updating so if the mouse p ([79342ce](CodeForked/react-tooltip@79342ce))

### Performance Improvements

* **Use sanitize-html-react instead of sanitize-html to avoid useless server side dependencies:** Us ([4b84caa](CodeForked/react-tooltip@4b84caa)), closes [ReactTooltip#424](https://github.com/CodeForked/react-tooltip/issues/424)

### BREAKING CHANGES

* Updating readme for demo
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 a pull request may close this issue.

6 participants