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

First cut at documentation of project arg/argTypes for discussion #16992

Merged
merged 3 commits into from Mar 22, 2022

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Dec 13, 2021

Issue: #11697

What I did

  • Added a section on project-level args to the args page
  • Created an example (perhaps not a very good one) of what you might use it for.

What we need to figure out:

  1. What's a good example of a project-level arg?
  2. Do we even want to talk about project-level args without figuring out and documenting (arg targets Args: Add ability to specific argType "targets" #16333)
  3. How can we document project-level argTypes? Should we be talking about story-level argTypes also?

@nx-cloud
Copy link

nx-cloud bot commented Dec 13, 2021

☁️ Nx Cloud Report

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

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@tmeasday tmeasday marked this pull request as draft December 13, 2021 05:18
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.

I think the preview.js example should be in snippets/common. Otherwise LGTM!

Also we should fix the implementation so that it works in both storyStoreV7 and legacy modes

@jonniebigodes
Copy link
Contributor

Looks good to me! And indeed the snippet should be placed in the common directory.

@@ -76,6 +76,20 @@ You can also define args at the component level; they will apply to all the comp

<!-- prettier-ignore-end -->

## Project args
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally like the clarity of "project" rather than "global" here, but we currently use the term "global" for parameters and decorators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! We changed this internally in 6.4, as it was super confusing to have "global args" and "globals" (which once upon a time were actually called globalArgs haha).

We should transition to using project everywhere I think. What's your recommendation in the meantime?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR should use "global". Then we can make another PR to update it to "project" everywhere, possibly only including that change in 6.5+.

@@ -10,7 +10,7 @@ Learn how and why to write stories in [the introduction](./introduction.md#using

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should add similar docs for argTypes to https://storybook.js.org/docs/react/api/argtypes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kylegach sounds reasonable to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I got a bit hung up on the fact that we don't actually document story-level argTypes there (I guess argTypes at the story level generally speaking don't make a heap of sense, although it can be done). But I'll add something.

@tmeasday
Copy link
Member Author

tmeasday commented Dec 20, 2021

OK, thanks all, I think this is ready now, perhaps pending #17043

@tmeasday tmeasday marked this pull request as ready for review December 20, 2021 04:11
@IanVS
Copy link
Member

IanVS commented Feb 9, 2022

Looks like #17043 was merged. Is this good-to-go now, then?

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.

@IanVS @tmeasday we still need to move the snippet to the common directory

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #16992 (3dcff54) into next (3e5d660) will not change coverage.
The diff coverage is n/a.

❗ Current head 3dcff54 differs from pull request most recent head 03279e0. Consider uploading reports for the commit 03279e0 to get more accurate results

@@           Coverage Diff           @@
##             next   #16992   +/-   ##
=======================================
  Coverage   32.23%   32.23%           
=======================================
  Files         939      939           
  Lines       18405    18405           
  Branches     3836     3836           
=======================================
  Hits         5932     5932           
  Misses      11997    11997           
  Partials      476      476           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e5d660...03279e0. Read the comment docs.

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

Looks good to me @tmeasday! Thanks for taking care of the snippets. Going to merge this @shilman !

@jonniebigodes jonniebigodes merged commit 5cacd0e into next Mar 22, 2022
@jonniebigodes jonniebigodes deleted the 11697-document-project-arg-argTypes branch March 22, 2022 18:07
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.

None yet

5 participants