Addon-controls: Conditional arg disabling #13890
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shilman
requested review from
ndelangen,
theinterned and
tmeasday
as code owners
shilman
added
addon: controls
args
maintenance
|
Does this have meaning outside of args table? |
|
Not currently, but it could. Disabling args entirely could disable them from being serialized, or disable them from URL sync. WDYT @tmeasday @ghengeveld? Rationale for this PR: |
|
LGTM |
Closed
Vadorequest
approved these changes
Mar 2, 2021
shilman
requested review from
aaronmcadam,
alexandrebodin,
alterx,
CodeByAlex,
dangreenisrael,
danielduan,
igor-dv,
jbovenschen,
kazupon and
leonrodenburg
as code owners
shilman
changed the title
|
Feedback from @ndelangen - let's make |
7 tasks
Nx Cloud ReportCI ran the following commands for commit 72e4364. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch
Sent with 💌 from NxCloud. |
|
|
Why was this PR closed? This comment best describes why this is needed. |
|
@jorisw this PR was a POC. we haven't decided whether to add this feature to controls and plan to revisit the decision based on the community response. |
notlee
added a commit
to Financial-Times/origami
that referenced
this pull request
Jan 10, 2022
Adds a `destroy` method to o-banner. This is used in the o-banner Story to clean up the close button when re-rendered, which o-banner dynamically creates on `init`. This is required to support the `suppressCloseButton` Story control. It appears banner headings (headingLong/headingShort) should only be used with the small/compact layout variants. Not with the default banner style which is full bleed. However it is possible to use Story controls to create that view. `@storybook/addon-knobs` (deprecated) allowed dynamic knobs, so we could hide the heading controls if the default layout was selected. That's not possible with `@storybook/addon-controls`: storybookjs/storybook#11984 I think that's probably a good thing. Support for dynamic controls was worked on but not merge. It's a poor experience when controls shift around. storybookjs/storybook#13890 For now this commit hides the layout control on layout demos, and hides the heading controls from the default layout demo, to avoid showing the discouraged heading + layout combination. However it is still possible to select the base layout with heading on the theme specific demos, so that the small/compact layout can also be selected which is allowed. This could be resolved by exporting 2 templates, one for each kind of banner / usecase. This could make components easier to reason with and maintain. For now this commit sticks to one banner template as coming up with a name without history / useage guidelines is difficult, and we don't know that users aren't already using headings with the base layout - though we never intended it as far as I can tell. The custom theme demo has not been migrated to a Story yet. https://registry.origami.ft.com/components/o-banner@4.2.0#demo-custom-theme I'm not sure there is value, and maybe harm, in showing made up customised styles alongside those with brand/design approval. Plus it's not clear how to re-create the style without understanding Sass and inspecting demo code. We probably want to have stories for customised components at a later date, with improved guidelines around them and demo Sass/JS pane. See related issue: #370 The description of the demo starts "Although not recommended for design consistency..." Let's not recomend it with a demo.
notlee
added a commit
to Financial-Times/origami
that referenced
this pull request
Jan 10, 2022
Destroy Method: Adds a `destroy` method to o-banner. This is used in the o-banner Story to clean up the close button when re-rendered, which o-banner dynamically creates on `init`. This is required to support the `suppressCloseButton` Story control. Hidden Controls: It appears banner headings (headingLong/headingShort) should only be used with the small/compact layout variants. Not with the default banner style which is full bleed. However it is possible to use Story controls to create that view. `@storybook/addon-knobs` (deprecated) allowed dynamic knobs, so we could hide the heading controls if the default layout was selected. That's not possible with `@storybook/addon-controls`: storybookjs/storybook#11984 I think that's probably a good thing. Support for dynamic controls was worked on but not merge. It's a poor experience when controls shift around. storybookjs/storybook#13890 For now this commit hides the layout control on layout demos, and hides the heading controls from the default layout demo, to avoid showing the discouraged heading + layout combination. However it is still possible to select the base layout with heading on the theme specific demos, so that the small/compact layout can also be selected which is allowed. This could be resolved by exporting 2 templates, one for each kind of banner / usecase. This could make components easier to reason with and maintain. For now this commit sticks to one banner template as coming up with a name without history / useage guidelines is difficult, and we don't know that users aren't already using headings with the base layout - though we never intended it as far as I can tell. No Custom Theme Demo: The custom theme demo has not been migrated to a Story yet. https://registry.origami.ft.com/components/o-banner@4.2.0#demo-custom-theme I'm not sure there is value, and maybe harm, in showing made up customised styles alongside those with brand/design approval. Plus it's not clear how to re-create the style without understanding Sass and inspecting demo code. We probably want to have stories for customised components at a later date, with improved guidelines around them and demo Sass/JS pane. See related issue: #370 No Custom Call To Action Demo: https://registry.origami.ft.com/components/o-banner@4.2.0#demo-custom-call-to-action-layout The description of the demo starts "Although not recommended for design consistency..." Let's not recomend it with a demo.
notlee
added a commit
to Financial-Times/origami
that referenced
this pull request
Jan 11, 2022
Destroy Method: Adds a `destroy` method to o-banner. This is used in the o-banner Story to clean up the close button when re-rendered, which o-banner dynamically creates on `init`. This is required to support the `suppressCloseButton` Story control. Hidden Controls: It appears banner headings (headingLong/headingShort) should only be used with the small/compact layout variants. Not with the default banner style which is full bleed. However it is possible to use Story controls to create that view. `@storybook/addon-knobs` (deprecated) allowed dynamic knobs, so we could hide the heading controls if the default layout was selected. That's not possible with `@storybook/addon-controls`: storybookjs/storybook#11984 I think that's probably a good thing. Support for dynamic controls was worked on but not merge. It's a poor experience when controls shift around. storybookjs/storybook#13890 For now this commit hides the layout control on layout demos, and hides the heading controls from the default layout demo, to avoid showing the discouraged heading + layout combination. However it is still possible to select the base layout with heading on the theme specific demos, so that the small/compact layout can also be selected which is allowed. This could be resolved by exporting 2 templates, one for each kind of banner / usecase. This could make components easier to reason with and maintain. For now this commit sticks to one banner template as coming up with a name without history / useage guidelines is difficult, and we don't know that users aren't already using headings with the base layout - though we never intended it as far as I can tell. No Custom Theme Demo: The custom theme demo has not been migrated to a Story yet. https://registry.origami.ft.com/components/o-banner@4.2.0#demo-custom-theme I'm not sure there is value, and maybe harm, in showing made up customised styles alongside those with brand/design approval. Plus it's not clear how to re-create the style without understanding Sass and inspecting demo code. We probably want to have stories for customised components at a later date, with improved guidelines around them and demo Sass/JS pane. See related issue: #370 No Custom Call To Action Demo: https://registry.origami.ft.com/components/o-banner@4.2.0#demo-custom-call-to-action-layout The description of the demo starts "Although not recommended for design consistency..." Let's not recomend it with a demo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #11984
What I did
Simplified disabling of individual args in ArgsTable, also disable controls conditionally on other controls:
Some examples:
How to test
See updated stories & unit tests