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

Docs: Update useSWC API reference #22984

Merged
merged 3 commits into from
Jun 12, 2023
Merged

Docs: Update useSWC API reference #22984

merged 3 commits into from
Jun 12, 2023

Conversation

kylegach
Copy link
Contributor

@kylegach kylegach commented Jun 8, 2023

What I did

Related to #22861, this PR updates the docs to be more API reference-centric.

  • Add to API reference
  • Update related content/snippets

How to test

  1. Follow the steps in the contributing instructions for this branch, api-reference-use-swc
  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 Jun 8, 2023
@kylegach kylegach self-assigned this Jun 8, 2023
@kylegach kylegach changed the title Update useSWC docs Docs: Update useSWC API reference Jun 8, 2023
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.

Just a couple of items we'll need to address, and this should be good to go. Appreciate the follow up on the pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kylegach, why was the flag removed? I don't see the point of removing this and leaving the other ones. What should happen instead is to leave the flags in and point them at the API documentation instead for visibility. as this is inadvertently opening up a door for questions/issues on why this is documented in other places and not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was removed because this API is not actually a feature flag. The others are.

I agree that the configure/overview page needs reworked now that api/main-config is available. I think that deserves its own, larger effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm aware that this is part of the framework options for the time being. But per my conversation with the stakeholder, I mentioned that this should be documented as an experimental flag here, and it was agreed that it should for the time being, hence why it was referenced as such. Additionally, having it here would surface it more, as it's basically only being referenced in either the Babel documentation or the API documentation, potentially opening up the door to an unwarranted documentation discussion/GitHub issue or Discord message.

@@ -14,11 +14,6 @@ const config: StorybookConfig = {
},
},
stories: ['../src/**/*.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been noticing a pattern of renaming snippets lately, and if you're ok, we'll need to figure out not only the naming convention and what goes in and what doesn't go in them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to document all of the conventions we're landing on for the docs, particularly the API references. I've waited until the patterns are proved out across more pages, first. Might be that time now, though...

For the snippet names, there's two reasons I changed this one:

  1. It used to be storybook-enable-swc-loader. Nearly all of our snippets pertain to Storybook, so a prefix of storybook- is unnecessary. Also, it's enabling the SWC compiler, not the loader.
  2. The API references (and their snippets) should contain the canonical docs information. Wherever reasonable, the guide pages should reuse and reference what is there. The snippet name reflects that.

@jonniebigodes
Copy link
Contributor

jonniebigodes commented Jun 8, 2023

Forgot one item, until we have confirmation that this feature is available on 7.0.x, this should not be labeled patch to avoid this accidentally being picked up and published on the website when the experimental feature is not available with the current stable version to avoid confusion.

cc @shilman

@kylegach kylegach removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 8, 2023
kylegach added 2 commits June 9, 2023 15:24
- Add to API reference
- Update related content/snippets
- To avoid it being part of the heading anchor
    - Helps links to anchor stay stable over time, when label is removed
@kylegach kylegach force-pushed the api-reference-use-swc branch from dbd4c26 to 852cc70 Compare June 9, 2023 21:24
@kylegach kylegach merged commit 49ad2a2 into next Jun 12, 2023
@kylegach kylegach deleted the api-reference-use-swc branch June 12, 2023 22:13
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.

2 participants