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

Use withSafeTimeout in NUX tips #7544

Merged
merged 2 commits into from Jun 27, 2018

Conversation

Projects
None yet
2 participants
@noisysocks
Member

noisysocks commented Jun 26, 2018

Description

Attempt at addressing #7461.

There's a chance that the DotTip component will have unmounted by time the defer callback executes. We can address this by using withSafeTimeout.

See #7461 (comment) for some useful context.

How has this been tested?

  1. Toggle tips off and then on
  2. Create a new post
  3. The first tip should be correctly positioned next to the inserter icon

noisysocks added some commits Jun 26, 2018

Use withSafeTimeout to fix position of DotTip
Using withSafeTimeout ensures that there is no exception thrown if the
component is unexpectedly unmounted.

We also make withSafeTimeout use createHigherOrderComponent so that
the affected snapshot tests have meaningful component names.
Fix withSafeTimeout documentation
withSafeTimeout gives the component a setTimeout prop, not a
setSafeTimeout prop.

@noisysocks noisysocks requested a review from aduth Jun 26, 2018

@aduth

aduth approved these changes Jun 26, 2018

Can we also create follow-up issues for (a) understanding what is causing the dot tips to be mounted and then unmounted and (b) resolving the issue with popover positioning recalculating (i.e. avoid the need for setTimeout altogether).

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 27, 2018

understanding what is causing the dot tips to be mounted and then unmounted

Are we sure that this happens? Noting that #7461 ended up being caused by a different issue.

resolving the issue with popover positioning recalculating (i.e. avoid the need for setTimeout altogether).

We discussed this a little in #7071. Would love your thoughts there—feel free to re-open the issue 🙂

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 27, 2018

Merging this even though it doesn't address #7461 because I reckon it doesn't hurt to be extra safe.

@noisysocks noisysocks merged commit 9ad4e87 into master Jun 27, 2018

2 checks passed

codecov/project 46.77% (+0.01%) compared to b454f1d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the fix/use-withSafeTimeout-in-tips branch Jun 27, 2018

@noisysocks noisysocks added this to the 3.2 milestone Jun 27, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jun 28, 2018

I think #7461 may have had multiple problems combined within the same issue. FWIW, there was a report of failing E2E from the .refresh call at #6956 (comment), even after the "fix" of #7461 (#7493).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment