-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Allow custom argType sorting #28565
base: next
Are you sure you want to change the base?
Allow custom argType sorting #28565
Conversation
28f3eee
to
8a18f23
Compare
controls: { | ||
sort: (a, b) => | ||
a.table?.category?.localeCompare(b.table?.category) || | ||
(a.type?.required === b.type?.required ? 0 : a.type?.required ? 1 : -1) || | ||
0, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (blocking): @kylegach @jonniebigodes just asking you both because GitHub suggests you both
I tested to pass a callback function here, however const { expanded, sort, presetColors } = useParameter<ControlsParameters>(PARAM_KEY, {});
then returns undefined
for sort
.
I also tried to pass e.g. 'custom'
as string, and then it successfully pass through as is. My assumption is that somewhere runtime-functions are not serializable and results in undefined
.
Is someone knowing what that could be and point me in the right direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shinigami92 parameters are loaded into Storybook's "preview" (the inner iframe that renders the stories). They are then serialized across a channel into the "manager" (the outer iframe that's Storybook's actual UI) using a library called telejson
.
Historically, telejson serialized pure functions by default. But then we started getting complaints about the use of eval
, which is forbidden in some companies. Now function serialization is opt-in using channelOptions.allowFunction. However, I think we might even remove that in SB9 because the eval
code still exists in telejson
which triggers some security checks.
If you want a function to be available in the server, your best bet is to have users set it in .storybook/manager.js
, see the renderLabel
example (which strangely doesn't seem to be documented anywhere else @kylegach @jonniebigodes). The manager APIs are sorely underdeveloped (something that @ndelangen wants to do something about, but which is currently lower priority) and in your case I think you really want the function to be available in both the manager AND the preview and we really don't have a great solution for that.
I'm open to suggestions for better ways to approach this -- it's definitely an area that could use work. We're talking about some architectural changes next year that could solve this in a very different way. But nothing in the short term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shilman OH 😲 this makes a lot of sense!
As you said, I could try to allow to register a function in manager and then just pass the registered function-name as string. And then see if that can be used.
If not, then I might need to wait until SB-9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen another good use case for the annotation server
8a18f23
to
11f76e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: @shilman how is it possible to somehow access inside ControlsPanel
a custom provided function? (I mean one that wont be a string like '(a,b)=>a.compare(b)'
, and then put that into eval(myFn)
, because this would be hilarious 😆)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shilman @kylegach @jonniebigodes any ideas?
Should we potentially think about to provide argTypes with an order property? e.g. |
closes #27520, closes #24524, related-to #25386
What I did
Allow custom sorting of
argTypes
via a function callbackChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
TODO
Manual testing
TODO
Documentation
TODO
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/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>