-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Core: Hoist 'control.options', validate them in core and introduce 'control.labels' #14169
Conversation
…tions' or 'options' (in that order)
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.
This looks fantastic. We need to update the controls documentation also!
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.
Looks great @ghengeveld
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.
Looks great to me, modulo a few minor suggestions.
I'm also wondering whether we should have a link from the controls docs back into MIGRATION.md
for existing controls users who read the updated docs and are confused?
@jonniebigodes can you take a look?
docs/snippets/common/component-story-custom-args-mapping.js.mdx
Outdated
Show resolved
Hide resolved
lib/client-api/src/inferControls.ts
Outdated
validator: (obj: any) => Array.isArray(obj), | ||
control: { | ||
type: 'object', | ||
validator: (obj: any) => Array.isArray(obj), |
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.
is validator
documented?
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.
Looks like you didn't document it when you added it.
I think this isn't used anymore, I think I removed it when we introduced the JSON editor.
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.
LOL almost zero memory of this 😭
Per our slack discussion, let's get rid of validators and also use the JSON editor UI for ALL arrays moving forward
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.
Looking great!
@shilman just did and it looks fantastic. I'm good with it. |
Issue: #13888
What I did
This gracefully replaces
control.options
with top-leveloptions
on argTypes. Top-leveloptions
are validated by the story store rather than the Controls addon. Contrary tocontrol.options
,options
only accepts an array of primitives. Labels can be customized usingcontrol.labels
.How to test
If your answer is yes to any of these, please make sure to include it in your PR.