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

Change forceReRender story to forceRemount #20752

Merged
merged 10 commits into from
Jan 31, 2023
Merged

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Jan 23, 2023

What I did

We have an internal CI story that tests that components remount when forced to - by asserting that a button looses focus after the event. However it was actually emitting FORCE_RE_RENDER, while actually it is the FORCE_REMOUNT that should trigger that behavior. Which is why I've changed it.

I've also enabled Vue for the tests, as that use case was recently fixed by #20712

How to test

See all sandboxes in Chromatic should actually be correct now.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@JReinhold JReinhold added build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job. labels Jan 23, 2023
@JReinhold JReinhold self-assigned this Jan 23, 2023
@JReinhold
Copy link
Contributor Author

@tmeasday this was naive of me. emitting FORCE_REMOUNT will immediately replay the play function, making it loop forever both in Storybook and Chromatic. What should we do here?

  1. Remove the test completely - it wouldn't be a huge loss since it has never worked, but on the other hand it would be nice to know that remounting works in the renderers.
  2. pass an option like skipPlay when emitting the event, and pass that all the way through to StoryRender.render(). I'm not sure this will work, or if it will break the whole flow in another way.
  3. ... other suggestions.

@tmeasday
Copy link
Member

tmeasday commented Jan 25, 2023

I don't think we should do anything fundamental to make the story work (if that's what you were suggesting in 2.

But can we do something a little hackier? Like only run the play function once per second or something like that?

I'm not sure if the story remounting is going to cause a problem for chromatic anyway perhaps, but at least this way we could test it manually (i.e we might need to disable chromatic).

@JReinhold
Copy link
Contributor Author

@tmeasday I've been through every single published Storybook now, and they all work as expected: alternating between focus and blur every 3 seconds. The story is disabled in both Chromatic and the test runner.

I think we should just merge this in and solve the Chromatic issue separately when we feel that is a priority.

@JReinhold JReinhold requested review from tmeasday and yannbf January 30, 2023 07:36
@JReinhold JReinhold merged commit 0e33781 into next Jan 31, 2023
@JReinhold JReinhold deleted the jeppe/fix-remount-story branch January 31, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates ci:daily Run the CI jobs that normally run in the daily job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants