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.

Sign up for GitHub

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

Jump to bottom

Add main configuration API reference pages #22539

Merged
merged 8 commits into from
May 24, 2023
Merged

Add main configuration API reference pages #22539

merged 8 commits into from
May 24, 2023

Conversation

kylegach
Copy link
Contributor

@kylegach kylegach commented May 12, 2023 •
edited
Loading

Linked to #17243

Related discussion: #22480 (comment)

What I did

  • Add an overview page for the main configuration object
  • Add a page for each property of the main configuration object (e.g. framework, stories, addons, etc.)
  • Rename TOC item @storybook/blocks → Doc Blocks, for consistency
  • Update snippets & guides related to main configuration API reference pages

Screenshot of main configuration overview page

How to test

  1. Follow the steps in the contributing instructions for this branch, api-reference-main-config
    • There's a lot of snippets referenced, so this step is critical!
  2. Check for:
    a. Completeness
    - Is every property documented fully?
    b. Correctness
    - Types
    - Required or not
    - Default values
    - Descriptions
    - Example snippets
    c. Consistency in structure

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"]

@kylegach kylegach added documentation patch:yes Bugfix & documentation PR that need to be picked to main branch labels May 12, 2023
@kylegach kylegach self-assigned this May 12, 2023
kylegach
kylegach commented May 12, 2023
View reviewed changes
docs/api/main-config-preview-body.md

</div>

For example, you can conditionally add scripts or styles, depending on the environment:
Copy link
Contributor Author

@kylegach kylegach May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now, both previewBody and previewHead use the same example. I think this analytics example is more appropriate for previewHead. Can anyone think of a different example for previewBody?

kylegach
kylegach commented May 12, 2023
View reviewed changes
docs/api/main-config.md Outdated Show resolved Hide resolved
kylegach
kylegach commented May 12, 2023
View reviewed changes
docs/toc.js
title: 'Main configuration',
pathSegment: '',
type: 'menu',
children: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These (along with every list of anything within the API references) are ordered like so:

  1. Overview
  2. Required properties
  3. Optional properties
  4. Experimental properties
  5. Deprecated properties

And each of those are then ordered alphabetically.

@kylegach kylegach force-pushed the api-reference-main-config branch from eb049c3 to 663e37e Compare May 12, 2023 19:10
@kylegach kylegach marked this pull request as ready for review May 12, 2023 19:10
@kylegach kylegach requested review from jonniebigodes and shilman May 12, 2023 19:10
@domyen
Copy link
Member

domyen commented May 12, 2023

For people googling for "main.js config" or "main.ts config", I don't think the keywords in the headings would come through as obvious.

Perhaps calling that out in a separate section would help:

  • Add an h2 before the snippet that says "main.js or main.ts"
  • Below that section, pull the description of "Configuration for Storybook is defined in .storybook/main.js|ts, which is located relative to the root of your project. A typical..."

In the sidebar:

  • "Main configuration" » "main.js|ts configuration" or just "main.js|ts"

@kylegach kylegach force-pushed the api-reference-main-config branch from 663e37e to 4c2e2bf Compare May 12, 2023 19:26
@kylegach
Copy link
Contributor Author

@domyen —

For people googling for "main.js config" or "main.ts config", I don't think the keywords in the headings would come through as obvious.

Here's some my thinking on the H1:

"main.js|ts configuration" is the most correct/expected, but looks terrible because of the |. That pipe also prevents a search for main.ts from matching in Algolia (and probably Google).

"main.js configuration" or "main.ts configuration" too heavily implies that only one of the file extensions is accepted and the one not-used is then not searchable.

Thus, the helpfully ambiguous "Main configuration".

Perhaps calling that out in a separate section would help:

Add an h2 before the snippet that says "main.js or main.ts"
Below that section, pull the description of "Configuration for Storybook is defined in .storybook/main.js|ts, which is located relative to the root of your project. A typical..."

Y'know, I thought that heading might look a little strange. It does, imo, but not nearly as much as I feared. I'll commit that.

In the sidebar:

  • "Main configuration" » "main.js|ts configuration" or just "main.js|ts"

I tried that:

The | in our font makes it hard to read, imo.

@domyen
Copy link
Member

domyen commented May 12, 2023

image

For the sidebar, how about "Main configuration" to "main.js or main.ts"

@kylegach
Copy link
Contributor Author

@domyen —

For the sidebar, how about "Main configuration" to "main.js or main.ts"

I still think "configuration" should be there.

@shilman shilman self-assigned this May 15, 2023
@domyen
Copy link
Member

domyen commented May 15, 2023 •
edited
Loading

Thanks for sharing that, I think the main.js|ts configuration is probably the most straightforward to me. Without the .js|ts I would not know it's a file.

wdyt @shilman?

@kylegach kylegach force-pushed the api-reference-main-config branch from b74e103 to 2501f57 Compare May 17, 2023 16:26
kylegach added a commit that referenced this pull request May 17, 2023
@kylegach
REMOVE BEFORE PUSHING (from #22539)
7c6c155
kylegach added a commit that referenced this pull request May 19, 2023
@kylegach
REMOVE BEFORE PUSHING (from #22539)
a2a6bb9
shilman
shilman reviewed May 22, 2023
View reviewed changes
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.

Amazing work @kylegach & sorry for the slow review. Commented a bit, but it's a huge PR so probably more to discuss.

docs/api/main-config-babel.md Outdated Show resolved Hide resolved
docs/api/main-config-babel.md Outdated Show resolved Hide resolved
docs/api/main-config-config.md Show resolved Hide resolved
docs/api/main-config-framework.md Outdated Show resolved Hide resolved
docs/api/main-config-preview-annotations.md Show resolved Hide resolved
docs/api/main-config-stories.md Show resolved Hide resolved
docs/api/main-config-stories.md Show resolved Hide resolved
docs/api/main-config-typescript.md Outdated Show resolved Hide resolved
docs/api/main-config-typescript.md Show resolved Hide resolved
docs/api/main-config-typescript.md Show resolved Hide resolved
@kylegach
Copy link
Contributor Author

Related discussion: #22629 (comment)

kylegach
kylegach commented May 23, 2023
View reviewed changes
docs/toc.js Show resolved Hide resolved
kylegach added 8 commits May 23, 2023 14:15
@kylegach
Rename TOC item: @storybook/blocks -> Doc blocks
7320a59 
- Always sort required properties first
@kylegach
Add /docs/api/main-config-* pages
e8a89db 
- Add relevant snippets
@kylegach
Update snippets & guides related to main-config-* pages
37144b7
@kylegach
Update Overview to ensure "main.js" and "main.ts" are searchable
06e8807
@kylegach
Simplify presentation of properties
92c7faa 
- # config.core -> # core
- ## `config.core.builder` -> ## `builder`
- Add "Parent: [main.js|ts configuration](./Overview.md)"
- Update TOC menu title
@kylegach
Update main.js|ts snippets for consistency
8cac168 
- Use `main-config-<property>-description` format, if possible
- Use "your-framework" wherever possible
- Always include `framework` & `stories` properties
- Remove unnecessary properties
@kylegach
Add examples for all main config sub properties
1d00b24
@kylegach
Address comments:
1d3262e 
- Add `babelDefault` and `managerHead` pages
- Fix broken links
- Clarify when some properties are primarily used by addon authors
- Document simplified options for `babel`, `babelDefault`, `viteFinal`, and `webpackFinal`
- Add description for `core.channelOptions`
- Remove the `previewMainTemplate` page
- Document possible shortcomings of some `stories` configurations
- Document limitations of some `typescript` options
@kylegach kylegach force-pushed the api-reference-main-config branch from 96a0163 to 1d3262e Compare May 23, 2023 20:16
jonniebigodes
jonniebigodes requested changes May 24, 2023
View reviewed changes
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.

One small item to address, and this should be good to go. Let me know and we'll go from there.

docs/snippets/common/main-config-framework.js.mdx Show resolved Hide resolved
jonniebigodes
jonniebigodes requested changes May 24, 2023
View reviewed changes
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.

One small item to address, and this should be good to go. Let me know and we'll go from there.

@jonniebigodes jonniebigodes merged commit a7b76e9 into next May 24, 2023
@jonniebigodes jonniebigodes deleted the api-reference-main-config branch May 24, 2023 14:51
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 26, 2023
shilman pushed a commit that referenced this pull request May 26, 2023
@jonniebigodes @shilman
Merge pull request #22539 from storybookjs/api-reference-main-config
9b7141a 
Add main configuration API reference pages
shilman added a commit that referenced this pull request May 26, 2023
@shilman
Merge pull request #22818 from storybookjs/main-prerelease
e7e37e8 
Merge pull request #22539 from storybookjs/api-reference-main-config
@dannyhw dannyhw mentioned this pull request Feb 5, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndelangen ndelangen ndelangen left review comments

@shilman shilman shilman left review comments

@jonniebigodes jonniebigodes jonniebigodes approved these changes

Assignees

@kylegach kylegach

@shilman shilman

Labels
documentation patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
Learning Team
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kylegach @domyen @shilman @ndelangen @jonniebigodes