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

CSF: Allow overridding globals at the story level #26654

Draft
wants to merge 8 commits into
base: next
Choose a base branch
from

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Mar 27, 2024

Closes #23347

What I did

  • Add globalOverrides to CSF (not in the official package yet)
  • Use to override globals when a story is selected
  • Add a concept of userGlobals (un-overridden globals), include that value in the UPDATE_GLOBALS event
  • Update the manager to allow access to userGlobals
  • Update toolbars addon to disable a toolbar when the global is overridden.

TODO:

  • CSF types
  • Tests
  • Other addons:
    • Backgrounds + Viewports use globals to control state - probably makes sense to allow overriding and disable the toolbar.
    • Controls use globals for conditional controls (globals affect controls), but cannot change global from a control, so no need to disable anything.
    • Measure + Outline use global for enabled state -- does it make sense to allow a story to force enabled?
  • Docs
    • General concept
    • Toolbars
    • Viewports?
    • Backgrounds?
    • Other addons?

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

  1. Run a sandbox
  2. Visit localhost:6006/?path=/story/lib-preview-api-globals--overrides to see basics
  3. Visit http://localhost:6006/?path=/story/addons-toolbars-globals--override to see toolbar behaviour
  4. Try changing global, reloading SB, etc

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>

Copy link

nx-cloud bot commented Mar 27, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6539f37. 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

Sent with 💌 from NxCloud.

@tmeasday tmeasday changed the title Allow overridding globals at the story level CSF: Allow overridding globals at the story level Mar 27, 2024
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.

This is great! We might want to do the other addons in a separate PR or separate PRs to scrutinize each one a little more.

code/addons/toolbars/src/components/ToolbarMenuList.tsx Outdated Show resolved Hide resolved
@@ -79,6 +82,7 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle(
>
<ToolbarMenuButton
active={isTooltipVisible || hasGlobalValue}
disabled={isOverridden}
Copy link
Member

Choose a reason for hiding this comment

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

Readonly might be a better UX. See this recent PR

#26577

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we leave it to someone else (w/ @cdedreuille's help) to create a readonly version of the TooltipLinkList & ToolbarMenuButton? They doesn't appear to exist at the moment.

Co-authored-by: Michael Shilman <shilman@users.noreply.github.com>
const [isTooltipVisible, setIsTooltipVisible] = useState(false);

const currentValue = globals[id];
const hasGlobalValue = !!currentValue;
const isOverridden = !deepEqual(currentValue, userGlobals[id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

a deep equal check might be problematic for cases where functions or class instances are assigned to global values, leading to potential circular references.

Why can't we check whether userGlobals[id] is present? This matches my expectation of an "override". Otherwise, we should also rename the variable to something like isDeepEqual.

Suggested change
const isOverridden = !deepEqual(currentValue, userGlobals[id]);
const isOverridden = id in userGlobals;

Copy link
Member Author

@tmeasday tmeasday Mar 29, 2024

Choose a reason for hiding this comment

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

@valentinpalkovic:

a deep equal check might be problematic for cases where functions or class instances are assigned to global values

globals need to be serializable to go over the channel, so I don't think your examples are possible. But there may be others we should consider.

Why can't we check whether userGlobals[id] is present?

So I think you have this backwards (this might be a sign that the naming is bad) - userGlobals are the globals the user has specified via clicking toolbars etc. globals = { ...userGlobals, ...globalOverrides }. So this is how we check if a globalOverride is set for a key (does globals[key] == userGlobals[key]).

Now I think about it, maybe we've made it difficult to do the thing that you generally would want to do (know if a value has been overrdden). Perhaps instead we should set { globals, globalOverrides } on the event, or even { globals, globalOverrides, userGlobals }?

WDYT @shilman? The downside is quite a bit of redundancy on the data sent on the channel.

code/lib/manager-api/src/index.tsx Outdated Show resolved Hide resolved
Co-authored-by: Valentin Palkovic <valentin@chromatic.com>
Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Fwiw, I'm good with the globalOverrides name. It's perhaps a bit clunky, relative to globals, but I think it's necessary to communicate that the behavior is different.

@tmeasday — Should this PR contain docs?

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

4 participants