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

Official example: try moving options to manager.js #9323

Merged
merged 14 commits into from
Mar 2, 2020
Merged

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Jan 4, 2020

I tried applying https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#using-managerjs to our own example storybook

I found two issues:

  1. showRoots doesn't work
  2. manager.js doesn't hot reload: e.g. if you change it to theme: themes.dark, it applies only after page reload

@vercel
Copy link

vercel bot commented Jan 4, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/p8c4py0u9
✅ Preview: https://monorepo-git-manager-options.storybook.now.sh

@Hypnosphi Hypnosphi added ci: do not merge maintenance User-facing maintenance tasks labels Jan 4, 2020
@Hypnosphi Hypnosphi requested a review from shilman January 4, 2020 18:30
@ndelangen
Copy link
Member

For 6.0.0 the documentation is correct.

For 5.3 we need to reverse this documentation a bit, to tell users to keep setting the global parameter

@ndelangen ndelangen self-assigned this Jan 7, 2020
@ndelangen ndelangen added this to the 6.0.0 milestone Jan 22, 2020
@ndelangen
Copy link
Member

I can confirm that:

showRoots doesn't work

I added a FIX in api/modules/stories.ts

@ndelangen
Copy link
Member

ndelangen commented Jan 28, 2020

@tmeasday I did find what @Hypnosphi found too:

When a user would make migrate all options to manager.js certain things break.

Also regarding theming, I have a PR open for 5.3 to document how theming + addon-docs is special: #9661

@Hypnosphi WDYT?

after this PR, this becomes perfectly valid config:

// manager.js
import { addons } from '@storybook/addons';
import { themes } from '@storybook/theming';

addons.setConfig({
  showRoots: true,
  theme: themes.light, // { base: 'dark', brandTitle: 'Storybook!' },
  storySort: (a, b) =>
    a[1].kind === b[1].kind ? 0 : a[1].id.localeCompare(b[1].id, undefined, { numeric: true }),
});

@ndelangen
Copy link
Member

manager.js not hot-module-reloading is fine by me for now. I don not consider this an critical important feature. I'd be nice to add in the future.

@@ -229,7 +230,7 @@ const initStoriesApi = ({
hierarchyRootSeparator: rootSeparator = undefined,
hierarchySeparator: groupSeparator = undefined,
showRoots = undefined,
} = (parameters && parameters.options) || {};
Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen do you think we should come up with a principled way to pull configuration from both parameters and .getConfig()? It seems like this comes up a bit and maybe there should be a single API for it?

Copy link
Member

Choose a reason for hiding this comment

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

For instance @atanasster does something very similar in #9095

lib/api/src/modules/stories.ts Outdated Show resolved Hide resolved
# Conflicts:
#	examples/official-storybook/manager.js
warnRemovingHierarchySeparators();
if (usingShowRoots) warnUsingHierarchySeparatorsAndShowRoots();
({ root, groups } = parseKind(kind, {
rootSeparator: rootSeparator || '|',
Copy link
Member

Choose a reason for hiding this comment

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

Are we removing these in 6.0? Or are we planning to patch this back to 5.3? Or will we do that in a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah removing those deprecations is on my wishlist

Copy link
Member

Choose a reason for hiding this comment

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

I know a single key on your keyboard that could make your wish come true 😉

Copy link
Member

Choose a reason for hiding this comment

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

I'm well aware of existence of the delete key on my keyboard

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2020-02-13 at 14 59 27

@ndelangen ndelangen merged commit 5caf02e into next Mar 2, 2020
@ndelangen ndelangen deleted the manager-options branch March 2, 2020 23:12
@tmeasday
Copy link
Member

tmeasday commented Mar 3, 2020

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants