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

Args: Add ability to specific argType "targets" #16333

Merged
merged 10 commits into from
Nov 8, 2021
Merged

Conversation

tmeasday
Copy link
Member

Issue: #16239

What I did

  • Added filter function in store
  • Implemented in React
  • Added stories to demonstrate

Still todo

  • Other frameworks
  • Put behind a feature flag somehow.

How to test

  • Is this testable with Jest or Chromatic screenshots?

Yes we have both.

@tmeasday tmeasday requested a review from shilman October 13, 2021 06:30
@nx-cloud
Copy link

nx-cloud bot commented Oct 13, 2021

Nx Cloud Report

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

@nx-cloud
Copy link

nx-cloud bot commented Oct 13, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit 8103861.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@shilman shilman changed the title Add ability to specific argType "targets" Args: Add ability to specific argType "targets" Oct 13, 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.

Looks great. Do we want to feature flag this?

@tmeasday
Copy link
Member Author

Yep! What do you think about the other frameworks? Should I do them too?

@shilman
Copy link
Member

shilman commented Oct 13, 2021

@tmeasday yes please! sorry I missed that comment in your PR description 👍

@tmeasday
Copy link
Member Author

Ok, done. I haven't added stories for angular or server (or really tested them).

if (!Component) {
throw new Error(
`Unable to render story ${id} as the component annotation is missing from the default export`
);
}
return <Component {...args} />;

const renderedArgs = FEATURES.argTypeTarget ? noTargetArgs(context) : args;
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about adding argsByTarget to the context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

@shilman I reworked this, as discussed.

Now:

  1. It is a parameter (similar to passArgsFirst) rather than a FEATURE. This was just more convenient, I am open to turning it back into a feature if you prefer that.
  2. We filter args by default if the feature is on, also adding argsByTarget and fullArgs for convenience.

@tmeasday
Copy link
Member Author

tmeasday commented Nov 8, 2021

@shilman I made the changes you requested.

@shilman shilman added this to the 6.4 PRs milestone Nov 8, 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.

Love it! 😻

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.

5 participants