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

feat: Add Stories for o-banner demos #523

Merged
merged 9 commits into from
Jan 11, 2022
Merged

feat: Add Stories for o-banner demos #523

merged 9 commits into from
Jan 11, 2022

Conversation

notlee
Copy link
Contributor

@notlee notlee commented 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.

ok: default layout story
Screenshot 2022-01-10 at 15 00 07

bad: theme story, with layout changed to the default layout via a control
(the default layout shouldn't be used with a heading)
Screenshot 2022-01-10 at 15 00 36

@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/[email protected]#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/[email protected]#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 notlee changed the title backstage: wip add o-banner stories feat: Add Stories for o-banner demos 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/[email protected]#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/[email protected]#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 notlee marked this pull request as ready for review January 10, 2022 15:36
@notlee notlee requested a review from a team as a code owner January 10, 2022 15:36
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--wn51ap January 10, 2022 15:42 Inactive
Copy link
Contributor

@JakeChampion JakeChampion left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes and left a suggestion and some questions

I deployed this to a review app but it looks to not work due to lots of responses returning 404s -- https://origami-webs-stories-o--wn51ap.herokuapp.com/storybook/

@notlee
Copy link
Contributor Author

notlee commented Jan 10, 2022

I deployed this to a review app but it looks to not work due to lots of responses returning 404s
🤔

...I deleted it, sorry

@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--ltp9mt January 10, 2022 17:16 Inactive
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--ltp9mt January 10, 2022 17:32 Inactive
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--ltp9mt January 10, 2022 17:34 Inactive
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--ltp9mt January 10, 2022 17:35 Inactive
As the "primaryAction" control is an object, it is not obvious to the user what options are available without a stand alone demo
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--b3wkgz January 11, 2022 10:42 Inactive
@notlee notlee requested a review from JakeChampion January 11, 2022 10:58
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--b3wkgz January 11, 2022 10:58 Inactive
@notlee notlee enabled auto-merge (rebase) January 11, 2022 11:16
@JakeChampion JakeChampion temporarily deployed to origami-webs-stories-o--b3wkgz January 11, 2022 11:17 Inactive
@notlee notlee merged commit e0c75ae into main Jan 11, 2022
@notlee notlee deleted the stories-o-banner branch January 11, 2022 11:21
@akomiqaia akomiqaia mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants