CSF: Update docs for using globals in viewports and background addons #28716

ndelangen · 2024-07-26T11:25:32Z

Related: #27705

What I did

I updated the docs to be in sync with the changes made here:
#26654

addon-backgrounds changed public interface
addon-viewport changed public interface
added new feature flags to enable that behavior
addon-toolbars changed public interface

I've requested @jonniebigodes's help with the following:
Adjust this PR in such a way that it becomes clear the new behavior is only so, when the featureFlags are turned on.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Updated documentation to reflect new API for configuring globals in viewports and backgrounds addons.

docs/_snippets/my-component-story-configure-viewports.md: Replaced viewports and defaultViewport with options and value, moved configuration to globals.
docs/_snippets/storybook-addon-backgrounds-configure-backgrounds.md: Changed background configuration to use an options object.
docs/_snippets/storybook-addon-backgrounds-disable-backgrounds.md: Updated backgrounds parameter from disable to disabled.
docs/api/main-config/main-config-features.mdx: Added viewportStoryGlobals and backgroundsStoryGlobals options.
code/package.json: Bumped vitest dependency from ^1.2.2 to ^1.6.0.

nx-cloud · 2024-07-26T11:30:02Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d220802. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

greptile-apps

LGTM

_{14 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

shilman

We need to make it more clear that these new docs require the feature flag to be set. There are docs for that but those feature flags but AFAICT they are completely disconnected from the addon docs

shilman · 2024-07-27T00:09:12Z

docs/essentials/backgrounds.mdx


-Default background color. Must match the `name` property of one of the [available colors](#values).
+Type: `Record<string, { name: string; value: string }>`


Should name be optional?

Do you have a reason for making it optional?

So that it can default to the key if unset

But why? Why have 2 ways to configure things?
By just having 1 way, it's clearer for us, clearer for users, less docs to write, less complexity in code, less potential for bugs.

I think we should enforce there to be a name prop, that's purely presentational and separate from the identifier.
If we support both, there's a good chance users won't even know about the name prop at all, and try to hack things in the key instead. Which could cause bugs, with character escaping.

I strongly believe we should strive for simplicity, rather than terseness.

shilman · 2024-07-27T00:11:01Z

docs/essentials/viewport.mdx

    name: string,
    styles: { height: string, width: string },
-    type: 'desktop' | 'mobile' | 'tablet',
-  },
+    type: 'desktop' | 'mobile' | 'tablet' | 'other',


What does 'other' refer to?

I don't know. but it was already there in the types.
Seemed correct to just repeat here what the types had.

It possibly is a value to indicate "don't display an icon at all"?

shilman · 2024-07-27T00:17:25Z

There are a bunch of fixes in this PR like disable => disabled. Can you please split those into a separate PR that we can patch back to the live docs?

jonniebigodes

@ndelangen, thank you for taking the time to put together this pull request and help document this new feature. I appreciate it 🙏 ! This is looking great. I left some small items for you to look into when you have a moment.

Also, one quick question for you. In the viewport example, you're using the isRotated: false option and I was wondering if this is a new available option, and if so, we'll need to include it in the corresponding table, alongside any other items that were introduced, so that users will know what's available. Wouldn't you agree?

docs/api/main-config/main-config-features.mdx

docs/essentials/backgrounds.mdx

docs/essentials/viewport.mdx

ndelangen · 2024-07-29T07:21:35Z

There are a bunch of fixes in this PR like disable => disabled. Can you please split those into a separate PR that we can patch back to the live docs?

Can you list all the ones you would want patched?
What does patched mean for documentation?
From what i understood from @jonniebigodes is that when this lands on next it replaces the current docs.

I've asked @jonniebigodes to find a good way to ensure this docs change lands in such a way it's clear the feature flag is required.

The example you listed is a implementation-change i made in the PR: #26654
AFAIK It cannot be patched back, because it's backwards incompatible.

ndelangen · 2024-07-29T07:45:16Z

Also, one quick question for you. In the viewport example, you're using the isRotated: false option and I was wondering if this is a new available option, and if so, we'll need to include it in the corresponding table, alongside any other items that were introduced, so that users will know what's available. Wouldn't you agree?

This is effectively the viewportRotated renamed, and placed into a single global:

storybook/code/addons/viewport/src/legacy/ToolLegacy.tsx

Line 180 in 4f9967d

const styles = getStyles(ref.current, item.styles, globals.viewportRotated);

Co-authored-by: jonniebigodes <joaocontadesenvolvimento@gmail.com>

…rybookjs/storybook into norbert/globals-docs-changes

… of globalTypes

shilman · 2024-07-29T18:43:57Z

@ndelangen

The example you listed is a implementation-change i made in the PR: #26654

OK I misunderstood the docs change. Please ignore that request.

docs/api/main-config/main-config-features.mdx

- Improve feature flag callouts - Add API details to migration notes and clarify examples - Restructure guides to use sub-heading structure instead of snippet tabs - Offers ability to explain differences between APIs more thoroughly - Also a better experience, because users' tab preference will not apply across snippets - Clarify example snippets - Only demonstate one concept per snippet - Adjust heading levels - Add globals API reference section - Improve snippet filenames - Remove redundant `storybook-` prefix - Prose tweaks

* next: Address feedback Address comments Typo Further updates Address feedback Remove TK Futher updates to #28716

- Improve feature flag callouts - Add API details to migration notes and clarify examples - Restructure guides to use sub-heading structure instead of snippet tabs - Offers ability to explain differences between APIs more thoroughly - Also a better experience, because users' tab preference will not apply across snippets - Clarify example snippets - Only demonstate one concept per snippet - Adjust heading levels - Add globals API reference section - Improve snippet filenames - Remove redundant `storybook-` prefix - Prose tweaks

ndelangen added 2 commits July 26, 2024 12:49

changes to backgrounds addon in relation to the globals work

acc974b

changes to viewport addon in relation to the globals work

ae7f432

ndelangen requested review from kylegach and jonniebigodes as code owners July 26, 2024 11:25

ndelangen assigned jonniebigodes Jul 26, 2024

ndelangen added documentation addon: viewport addon: backgrounds ci:docs Run the CI jobs for documentation checks only. labels Jul 26, 2024

Merge branch 'tom/23347-story-globals' into norbert/globals-docs-changes

f380221

greptile-apps bot reviewed Jul 26, 2024

View reviewed changes

shilman reviewed Jul 27, 2024

View reviewed changes

jonniebigodes requested changes Jul 28, 2024

View reviewed changes

docs/api/main-config/main-config-features.mdx Outdated Show resolved Hide resolved

docs/essentials/backgrounds.mdx Outdated Show resolved Hide resolved

docs/essentials/viewport.mdx Outdated Show resolved Hide resolved

ndelangen and others added 4 commits July 29, 2024 11:36

Merge branch 'tom/23347-story-globals' into norbert/globals-docs-changes

eda3f77

Update docs/api/main-config/main-config-features.mdx

0678a2a

Co-authored-by: jonniebigodes <joaocontadesenvolvimento@gmail.com>

apply requested changes to docs

0d7d389

Co-authored-by: jonniebigodes <joaocontadesenvolvimento@gmail.com>

Merge branch 'norbert/globals-docs-changes' of https://github.com/sto…

d64bff2

…rybookjs/storybook into norbert/globals-docs-changes

ndelangen requested a review from jonniebigodes July 29, 2024 09:57

ndelangen added 5 commits July 29, 2024 15:58

Merge branch 'tom/23347-story-globals' into norbert/globals-docs-changes

7525fc9

add mention of addonToolbarParameters feature flag

693b64d

remove defaultValue, change toolbars config to use parameters instead…

7534db5

… of globalTypes

adjust title

0ac41ab

Merge branch 'tom/23347-story-globals' into norbert/globals-docs-changes

7cd8de0

shilman reviewed Jul 29, 2024

View reviewed changes

docs/api/main-config/main-config-features.mdx Outdated Show resolved Hide resolved

Merge branch 'tom/23347-story-globals' into norbert/globals-docs-changes

045d723

jonniebigodes merged commit 9a3d6fc into next Aug 6, 2024
9 checks passed

jonniebigodes deleted the norbert/globals-docs-changes branch August 6, 2024 14:10

github-actions bot mentioned this pull request Aug 6, 2024

Release: Prerelease 8.3.0-alpha.5 #28815

Merged

29 tasks

kylegach mentioned this pull request Aug 9, 2024

Docs: Story globals improvements #28856

Merged

4 tasks

kylegach added a commit that referenced this pull request Aug 26, 2024

Merge branch 'next' into docs-addon-vitest

a556c3f

* next: Address feedback Address comments Typo Further updates Address feedback Remove TK Futher updates to #28716

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSF: Update docs for using globals in viewports and background addons #28716

CSF: Update docs for using globals in viewports and background addons #28716

ndelangen commented Jul 26, 2024 •

edited

Loading

nx-cloud bot commented Jul 26, 2024 •

edited

Loading

greptile-apps bot left a comment

shilman left a comment

shilman Jul 27, 2024

ndelangen Jul 29, 2024

shilman Jul 29, 2024 •

edited

Loading

ndelangen Jul 30, 2024

shilman Jul 27, 2024

ndelangen Jul 29, 2024

ndelangen Jul 30, 2024

shilman commented Jul 27, 2024

jonniebigodes left a comment

ndelangen commented Jul 29, 2024

ndelangen commented Jul 29, 2024

shilman commented Jul 29, 2024


		Default background color. Must match the `name` property of one of the [available colors](#values).
		Type: `Record<string, { name: string; value: string }>`

CSF: Update docs for using globals in viewports and background addons #28716

CSF: Update docs for using globals in viewports and background addons #28716

Conversation

ndelangen commented Jul 26, 2024 • edited Loading

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Greptile Summary

nx-cloud bot commented Jul 26, 2024 • edited Loading

☁️ Nx Cloud Report

greptile-apps bot left a comment

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

shilman Jul 27, 2024

Choose a reason for hiding this comment

ndelangen Jul 29, 2024

Choose a reason for hiding this comment

shilman Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

ndelangen Jul 30, 2024

Choose a reason for hiding this comment

shilman Jul 27, 2024

Choose a reason for hiding this comment

ndelangen Jul 29, 2024

Choose a reason for hiding this comment

ndelangen Jul 30, 2024

Choose a reason for hiding this comment

shilman commented Jul 27, 2024

jonniebigodes left a comment

Choose a reason for hiding this comment

ndelangen commented Jul 29, 2024

ndelangen commented Jul 29, 2024

shilman commented Jul 29, 2024

ndelangen commented Jul 26, 2024 •

edited

Loading

nx-cloud bot commented Jul 26, 2024 •

edited

Loading

shilman Jul 29, 2024 •

edited

Loading