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

bug: provide args values from url to control #16498

Closed

Conversation

pahan35
Copy link

@pahan35 pahan35 commented Oct 27, 2021

Issue: #15278

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 27, 2021

Nx Cloud Report

CI ran the following commands for commit 4fb913d. 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.

@tmeasday
Copy link
Member

Hey @pahan35 thanks for this!

Actually this is very similar to a PR I just implemented #16488, which covers off emitting the event on a different code path (HMR).

I think it would make sense to emit from the same place I did

if (implementationChanged) {
this.channel.emit(Events.STORY_ARGS_UPDATED, { storyId, args });
}
- except also in the case of persisted args.

I would also love to see a test for this. Thanks!

@pahan35
Copy link
Author

pahan35 commented Oct 28, 2021

Hey @pahan35 thanks for this!

Actually this is very similar to a PR I just implemented #16488, which covers off emitting the event on a different code path (HMR).

I think it would make sense to emit from the same place I did

if (implementationChanged) {
this.channel.emit(Events.STORY_ARGS_UPDATED, { storyId, args });
}

  • except also in the case of persisted args.
    I would also love to see a test for this. Thanks!

Great changes! Thanks for working on it. I'll close this one.

@pahan35 pahan35 closed this Oct 28, 2021
@pahan35 pahan35 deleted the fix/15278-take-args-state-from-url branch October 28, 2021 06:49
@tmeasday
Copy link
Member

Hey @pahan35 - I wasn't saying that you should close this, my changes won't help with the case you are fixing here (when we set args on load via persistedArgs). Would you like to make a new PR that builds off mine?

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.

3 participants