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

Use withSafeTimeout in NUX tips #7544

Merged
merged 2 commits into from
Jun 27, 2018
Merged

Conversation

noisysocks
Copy link
Member

@noisysocks 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

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.
withSafeTimeout gives the component a setTimeout prop, not a
setSafeTimeout prop.
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended [Feature] NUX Anything that impacts the new user experience labels Jun 26, 2018
@noisysocks noisysocks requested a review from aduth June 26, 2018 05:24
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member Author

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
@noisysocks noisysocks deleted the fix/use-withSafeTimeout-in-tips branch June 27, 2018 06:53
@noisysocks noisysocks added this to the 3.2 milestone Jun 27, 2018
@aduth
Copy link
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
Labels
[Feature] NUX Anything that impacts the new user experience [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants