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

Core: Fix args values updated from url to control #16508

Merged
merged 10 commits into from
Nov 7, 2021

Conversation

pahan35
Copy link

@pahan35 pahan35 commented Oct 28, 2021

Issue: #15278

Reopened from #16498

What I did

I passed values from URL to ArgsTable by emitting storyArgsUpdated with URL args state, which is not populated on the initial render.

How to test

Go to the issue and try to reproduce it with these changes.

If a unit test is necessary for this fix, let me know how to write it.

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

@nx-cloud
Copy link

nx-cloud bot commented Oct 28, 2021

Nx Cloud Report

CI ran the following commands for commit ded097e. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@pahan35
Copy link
Author

pahan35 commented Oct 29, 2021

@tmeasday what are the next steps to get it merged?

@pahan35
Copy link
Author

pahan35 commented Oct 29, 2021

@shilman any chance to get this PR merged?

@shilman
Copy link
Member

shilman commented Oct 29, 2021

@pahan35 i'd like to get @tmeasday to review this one before merging, but we should be able to get it into 6.4 if everything looks groovy. thanks so much for your hard work and patience!

@tmeasday
Copy link
Member

Hey @pahan35 -- so things I would like to see on this PR:

  1. A test that when a story is the first selection and args are set in the URL, the event is emitted. You could simply copy this test and check the event like in this test

  2. I think the emitting should happen in the renderSelection function, which is both the place that we (a) set the persisted (i.e. URL) args and emit the same event for HMR also.

@tmeasday
Copy link
Member

tmeasday commented Nov 3, 2021

Actually, looking at this more closely, I am not sure why this is necessary in the (6.4) store @pahan35?

Are you sure about this problem? Can you provide a reproduction? We don't render the docs element yet at the time you are emitting, so I cannot see how the emitting you are doing there would help.

@pahan35
Copy link
Author

pahan35 commented Nov 3, 2021

Actually, looking at this more closely, I am not sure why this is necessary in the (6.4) store @pahan35?

Are you sure about this problem? Can you provide a reproduction? We don't render the docs element yet at the time you are emitting, so I cannot see how the emitting you are doing there would help.

For a repro, you can use your repro repository.

There https://recordit.co/qq5aAHEsl5 is a recording with a mentioned problem.

As you see, there is primary:false in URL, and it's rendered correctly. However, its control is in the wrong state.

@pahan35
Copy link
Author

pahan35 commented Nov 4, 2021

Finally, I've managed how to update deps, fixed broken tests, and added a new one.

@tmeasday could you please have a look?

@tmeasday
Copy link
Member

tmeasday commented Nov 5, 2021

Thanks so much @pahan35!! I simplified things a little bit, let me know if you see any problems with it (tests still passing and my reproduction seems to be fixed still!)

@pahan35
Copy link
Author

pahan35 commented Nov 5, 2021

Thanks so much @pahan35!! I simplified things a little bit, let me know if you see any problems with it (tests still passing and my reproduction seems to be fixed still!)

It should be OK. It was a bit unclear to me what implemenrationChange is used for, so I haven't touched it.

Thanks for an improvement from your side!

@tmeasday
Copy link
Member

tmeasday commented Nov 6, 2021

@shilman this is good to merge if you are happy with it?

@shilman shilman changed the title bug: provide args values from url to control (reopen) Core: Fix args values to control from url to control Nov 6, 2021
@jonniebigodes
Copy link
Contributor

@pahan35 I've been keeping an eye on this pull request and one thing that I wanted to follow up with you! If you wouldn't mind jumping into our Discord Server and message me (same username)?

Have a great weekend!

Stay safe

@pahan35
Copy link
Author

pahan35 commented Nov 6, 2021

@pahan35 I've been keeping an eye on this pull request and one thing that I wanted to follow up with you! If you wouldn't mind jumping into our Discord Server and message me (same username)?

Have a great weekend!

Stay safe

I've sent a friend request.

I'll be on vacation for a week and will return back at November 15 to continue my work on this if there are some problems.

Thanks, everybody for your help!

@shilman shilman added this to the 6.4 PRs milestone Nov 7, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Nice one @pahan35 @tmeasday 🏅

@shilman shilman changed the title Core: Fix args values to control from url to control Core: Fix args values updated from url to control Nov 7, 2021
@shilman shilman merged commit 8fc84eb into storybookjs:next Nov 7, 2021
@jonniebigodes
Copy link
Contributor

@pahan35 I saw the notification and followed up with you, thanks for following up with me, appreciate it! Hope you have a great vacation!

Stay safe

@pahan35 pahan35 deleted the fix/15278-take-args-state-from-url branch November 15, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants