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

Docs: Show primary story description and headline in autodocs #20604

Merged
merged 16 commits into from
Jan 19, 2023

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jan 13, 2023

Issue: N/A

Telescoping on #20559

What I did

Added a render option forceInitialArgs which means a story is rendered with it's initial args no matter what and doesn't respond to arg updates. Use this in the <Stories/> block, and switch to showing the primary story there.

The idea is that now in Autodocs we show:

  • Primary story, reactive
  • Controls (to control ☝️)
  • All stories (inc. primary) with descriptions, unreactive.

image

How to test

  • Look at tests + stories
  • Try autodocs!

@tmeasday tmeasday requested a review from JReinhold January 13, 2023 06:51
@tmeasday tmeasday changed the title Refactor props of Story block Docs: Show primary story description and headline in autodocs Jan 13, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@tmeasday tmeasday marked this pull request as ready for review January 15, 2023 11:46
@tmeasday tmeasday requested a review from JReinhold January 15, 2023 11:46
@tmeasday
Copy link
Member Author

Ok, now this is ready for re-review. Sorry for the early review @JReinhold. I changed the name and added a test for the "initial" behaviour.

I wasn't sure if we wanted to add stories for the Stories and DocsStory components. I sort of feel like they'll be duplicative and it'll be hard to actually test it's working outside of more copies of the same play function.

@tmeasday tmeasday force-pushed the add-ignoreArgsUpdates branch from a8d6012 to aaeeb1a Compare January 15, 2023 11:49
@tmeasday
Copy link
Member Author

tmeasday commented Jan 16, 2023

@JReinhold so the e2e test is failing due to this query:

const autoplayPre = root.locator('#story--addons-docs-docspage-autoplay--autoplay pre');

I guess that's because the Story component generates a DOM id based on only the story's id, so the same id is getting generated twice. That's a no-no, right?:

<style>{`#story--${story.id} { min-height: ${height}; transform: translateZ(0); overflow: auto }`}</style>

What shall we do about this do you think?

@shilman
Copy link
Member

shilman commented Jan 16, 2023

I think the primary story should have a special ID. not sure what though. Not sure what though (<id>--primary?) or how it would impact anything

@tmeasday
Copy link
Member Author

tmeasday commented Jan 16, 2023

Ok, that makes sense. I did that.

@tmeasday
Copy link
Member Author

@valentinpalkovic the failure on this branch comes from the angular renderer which doesn't appear to be able to render the same story more than once on the screen:

image

Is that surprising to you? If it's expected would it be easy to change/fix?

@valentinpalkovic
Copy link
Contributor

Fixed in #20559 :)

@tmeasday
Copy link
Member Author

Oh amazing!

Base automatically changed from tom/sb-1144-update-api-of-story-block to next January 17, 2023 21:12
@JReinhold
Copy link
Contributor

@valentinpalkovic

Fixed in #20559 :)

@tmeasday

Are you sure about that Valentin? The issue you and I discussed a few days ago, was about Angular being unable to render any multiple stories twice, like it does in docs. But this here is specifically about rendering the same story twice.

Maybe you've already taken care of this since our last conversation, then please ignore me.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

code/ui/blocks/src/blocks/Story.stories.tsx Outdated Show resolved Hide resolved
@valentinpalkovic
Copy link
Contributor

@valentinpalkovic

Fixed in #20559 :)

@tmeasday

Are you sure about that Valentin? The issue you and I discussed a few days ago, was about Angular being unable to render any multiple stories twice, like it does in docs. But this here is specifically about rendering the same story twice.

Maybe you've already taken care of this since our last conversation, then please ignore me.

Angular components are rendered as Standalone components in https://github.com/storybookjs/storybook/pulls. Therefore, the same component can be rendered multiple times without any issues.

@tmeasday tmeasday changed the base branch from next to valentin/angular15-maintenance January 18, 2023 09:14
@tmeasday
Copy link
Member Author

I changed the base of this #20559, let's find out for sure!

@valentinpalkovic
Copy link
Contributor

@tmeasday Hmm. Still, it does not work. let me take a quick look. Shouldn't be that hard to fix.

Base automatically changed from valentin/angular15-maintenance to next January 18, 2023 14:03
@tmeasday tmeasday merged commit 305bf40 into next Jan 19, 2023
@tmeasday tmeasday deleted the add-ignoreArgsUpdates branch January 19, 2023 04:20
@chakAs3
Copy link
Contributor

chakAs3 commented Jan 20, 2023

In my opinion, The Main Story, Default or Playground should be generated as the Base Story from the component definition only ( with default values ) and be independent of all Stories.
Base/Default Story is the Interactive Playground that can match the state of all the stories and not only the first one.

I used the same technique in most Storybook projects

image

so the Playground on where you mix and match different states of your component, and mostly the following stories are complete illustrations of Variants so the user can see a max of the component's states.

i have been raising some issues since the update.

@JReinhold
Copy link
Contributor

In my opinion, The Main Story, Default or Playground should be generated as the Base Story from the component definition only ( with default values ) and be independent of all Stories. Base/Default Story is the Interactive Playground that can match the state of all the stories and not only the first one.

I used the same technique in most Storybook projects

image

so the Playground on where you mix and match different states of your component, and mostly the following stories are complete illustrations of Variants so the user can see a max of the component's states.

i have been raising some issues since the update.

I don't disagree with you, but in the current data model I don't think that's possible. There's no such thing as displaying a story only based on the meta information, only actual stories can be displayed.

And what would you expect to be shown with the following stories file?

export default {
  title: 'My Button'
}

export const Primary = {
  render: () => <button>hello</button>
}

Without a component or a render function on meta, we can't render anything solely based on the meta information.

@chakAs3
Copy link
Contributor

chakAs3 commented Jan 20, 2023

In my opinion, The Main Story, Default or Playground should be generated as the Base Story from the component definition only ( with default values ) and be independent of all Stories. Base/Default Story is the Interactive Playground that can match the state of all the stories and not only the first one.
I used the same technique in most Storybook projects
image
so the Playground on where you mix and match different states of your component, and mostly the following stories are complete illustrations of Variants so the user can see a max of the component's states.
i have been raising some issues since the update.

I don't disagree with you, but in the current data model I don't think that's possible. There's no such thing as displaying a story only based on the meta information, only actual stories can be displayed.

And what would you expect to be shown with the following stories file?

export default {
  title: 'My Button'
}

export const Primary = {
  render: () => <button>hello</button>
}

Without a component or a render function on meta, we can't render anything solely based on the meta information.

I know that's why i said generated Story we generate what we need to render the Story based on the Component + meta info
I need a component and args to render and if there is a Story context that will be great more info,

from your code, I expect a hello button

@GuillaumeNury
Copy link

@tmeasday hello! What is the motivation of changing the default value of includePrimary ?
I cannot find a way to change this value.

@JReinhold
Copy link
Contributor

JReinhold commented Feb 7, 2023

@tmeasday hello! What is the motivation of changing the default value of includePrimary ? I cannot find a way to change this value.

  1. It seemed less magical and more obvious to us that the list of Stories shows all stories, and didn't "hide" anything.
  2. Our new support for story-level descriptions (along side component-level descriptions) needed a place to show the description for the primary story, and showing it in the list with the rest of the stories made this a non-issue.

I believe you can get back the old behavior by providing your own custom DocsPage that largely copies the existing one:

https://storybook.js.org/docs/7.0/react/writing-docs/docs-page#write-a-custom-template

// .storybook/preview.jsx

import React from 'react';
import { Title, Subtitle, Description, Primary, Controls, Stories } from '@storybook/blocks';

export const parameters = {
  ...
  docs: {
    page: () => (
	  <>
	    <Title />
	    <Subtitle />
	    <Description />
	    <Primary />
	    <Controls />
	    <Stories includePrimary={false} />
	  </>
	);
  },
};

Do you have a specific use-case that requires the primary story to be hidden from the list of stories?

@GuillaumeNury
Copy link

Ok. We use Angular, I can see that I should dig around mdx file (doc here).
Our design system is documented using ZeroHeight. We display only one story with its controls on each page (exemple here which exposes this story). Currently, we hide additional stories using CSS 🙈

@GuillaumeNury
Copy link

@JReinhold I played with custom Docs template and it worked like a charm. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants