RFC(storybook): conventions for story organization, naming and design#19206
Conversation
|
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e820ea0:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: a49159c11491bb1750350f9b965b7f3638a62495 (build) |
📊 Bundle size reportUnchanged fixtures
|
| **Proposal:** | ||
|
|
||
| ### 8. dissecting big story files into smaller ones | ||
| 1. Every component can have at most one `.stories.tsx` file. This file has a default export with Storybook configuration for the docs page. |
There was a problem hiding this comment.
Do you have any vision to enforcing this requirement ? It should be quite simply to use multiple stories.tsx files and still not change the storybook structurally at all
There was a problem hiding this comment.
it's why I kinda liked the stories/ folder option I originally proposed, since we can force storybook to load folders from a specific folder instead of all wildcard blobs
There was a problem hiding this comment.
🤔 We could contact storybook maintainers and see if they’d be open to a PR which would enforce unique titles throughout .stories.tsx files, in an opt-in mode. I believe they could be open to it as they state in their documentation that titles should be unique. If titles are unique, duplicate .stories.tsx would be always shown as different pages on the doc site, so this would be easier to keep in check.
That’d be the vision. I am not sure if it’ll actually get done :/.
it's why I kinda liked the stories/ folder option I originally proposed, since we can force storybook to load folders from a specific folder instead of all wildcard blobs
Putting all stories into one folder doesn’t solve the issue though, right? In one folder, you could still have multiple .stories.tsx files for one component.
There was a problem hiding this comment.
Do you have any vision to enforcing this requirement ?
we can/want enforce guides via generators + lint/additional checks will need to be implemented manually...
but generators is primary point of HOW TO, as it if we decide to change things in future, based on that generator we can prepare migration generators and automate stuff
| TBA | ||
| ### 8. location and naming convention | ||
|
|
||
| 1. Story files should be in the same folder as the component which they are targeting. |
There was a problem hiding this comment.
Let's go into detail about the component which they are targeting
i.e. if I wanted to show off MenuItemCheckbox which can't function without Menu, should that go in the Menu folder or the MenuItemCheckbox folder ?
There was a problem hiding this comment.
You want to show off MenuItemCheckbox, so it should go into MenuItemCheckbox folder.
Is this better?
| 1. Story files should be in the same folder as the component which they are targeting. | |
| 1. Story files should be in the same folder as the component which they are meant to show off. This will usually be the component which is the most unique to the story. |
There was a problem hiding this comment.
maybe it would make sense to set following rule - only package public API should have 1:1 stories that are rendered in SB tree view, WDYT ?
There was a problem hiding this comment.
@Hotell - Are you suggesting that every publicly available component must have exactly one .stories.tsx file? I think at most one is a better rule, as for some subcomponents separate Storybook doc page is not necessary. Examples would be AccordionItem, MenuGroup, ...
There was a problem hiding this comment.
yeah what you said. there is another rabbit hole though :D , when you mentioned subcomponents - we probably should use generated controls for those as well, which will complicate things a lot ... but we can address that in another PR
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 891 | 900 | 5000 | |
| BaseButton | mount | 913 | 923 | 5000 | |
| Breadcrumb | mount | 2494 | 2507 | 1000 | |
| ButtonNext | mount | 436 | 430 | 5000 | |
| Checkbox | mount | 1574 | 1571 | 5000 | |
| CheckboxBase | mount | 1348 | 1322 | 5000 | |
| ChoiceGroup | mount | 4801 | 4785 | 5000 | |
| ComboBox | mount | 996 | 982 | 1000 | |
| CommandBar | mount | 9730 | 9755 | 1000 | |
| ContextualMenu | mount | 6001 | 5938 | 1000 | |
| DefaultButton | mount | 1137 | 1132 | 5000 | |
| DetailsRow | mount | 3662 | 3720 | 5000 | |
| DetailsRowFast | mount | 3639 | 3599 | 5000 | |
| DetailsRowNoStyles | mount | 3582 | 3546 | 5000 | |
| Dialog | mount | 2086 | 2064 | 1000 | |
| DocumentCardTitle | mount | 128 | 144 | 1000 | |
| Dropdown | mount | 3219 | 3221 | 5000 | |
| FluentProviderNext | mount | 6780 | 6703 | 5000 | |
| FocusTrapZone | mount | 1777 | 1749 | 5000 | |
| FocusZone | mount | 1805 | 1716 | 5000 | |
| IconButton | mount | 1800 | 1819 | 5000 | |
| Label | mount | 351 | 328 | 5000 | |
| Layer | mount | 1781 | 1731 | 5000 | |
| Link | mount | 457 | 471 | 5000 | |
| MakeStyles | mount | 1703 | 1702 | 50000 | |
| MenuButton | mount | 1467 | 1487 | 5000 | |
| MessageBar | mount | 1911 | 1947 | 5000 | |
| Nav | mount | 3215 | 3285 | 1000 | |
| OverflowSet | mount | 982 | 1001 | 5000 | |
| Panel | mount | 1972 | 1973 | 1000 | |
| Persona | mount | 805 | 799 | 1000 | |
| Pivot | mount | 1401 | 1420 | 1000 | |
| PrimaryButton | mount | 1316 | 1262 | 5000 | |
| Rating | mount | 7884 | 7706 | 5000 | |
| SearchBox | mount | 1372 | 1361 | 5000 | |
| Shimmer | mount | 2579 | 2528 | 5000 | |
| Slider | mount | 1907 | 1935 | 5000 | |
| SpinButton | mount | 5268 | 4889 | 5000 | |
| Spinner | mount | 403 | 415 | 5000 | |
| SplitButton | mount | 3174 | 3426 | 5000 | |
| Stack | mount | 503 | 505 | 5000 | |
| StackWithIntrinsicChildren | mount | 1560 | 1599 | 5000 | |
| StackWithTextChildren | mount | 4797 | 4716 | 5000 | |
| SwatchColorPicker | mount | 10128 | 10259 | 5000 | |
| Tabs | mount | 1409 | 1380 | 1000 | |
| TagPicker | mount | 2594 | 2556 | 5000 | |
| TeachingBubble | mount | 11381 | 11389 | 5000 | |
| Text | mount | 424 | 426 | 5000 | |
| TextField | mount | 1417 | 1362 | 5000 | |
| ThemeProvider | mount | 1152 | 1165 | 5000 | |
| ThemeProvider | virtual-rerender | 565 | 570 | 5000 | |
| Toggle | mount | 808 | 808 | 5000 | |
| buttonNative | mount | 114 | 116 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| GridMinimalPerf.default | 360 | 328 | 1.1:1 |
| AlertMinimalPerf.default | 283 | 262 | 1.08:1 |
| CardMinimalPerf.default | 605 | 562 | 1.08:1 |
| SegmentMinimalPerf.default | 368 | 340 | 1.08:1 |
| ImageMinimalPerf.default | 408 | 382 | 1.07:1 |
| RadioGroupMinimalPerf.default | 483 | 453 | 1.07:1 |
| VideoMinimalPerf.default | 666 | 623 | 1.07:1 |
| AvatarMinimalPerf.default | 203 | 192 | 1.06:1 |
| BoxMinimalPerf.default | 358 | 339 | 1.06:1 |
| DividerMinimalPerf.default | 383 | 363 | 1.06:1 |
| HeaderMinimalPerf.default | 387 | 366 | 1.06:1 |
| LabelMinimalPerf.default | 400 | 379 | 1.06:1 |
| TextAreaMinimalPerf.default | 543 | 513 | 1.06:1 |
| AnimationMinimalPerf.default | 421 | 400 | 1.05:1 |
| ChatDuplicateMessagesPerf.default | 308 | 293 | 1.05:1 |
| SkeletonMinimalPerf.default | 367 | 354 | 1.04:1 |
| AttachmentSlotsPerf.default | 1099 | 1064 | 1.03:1 |
| ChatWithPopoverPerf.default | 364 | 353 | 1.03:1 |
| ItemLayoutMinimalPerf.default | 1265 | 1231 | 1.03:1 |
| LayoutMinimalPerf.default | 375 | 365 | 1.03:1 |
| ListWith60ListItems.default | 643 | 623 | 1.03:1 |
| MenuButtonMinimalPerf.default | 1714 | 1661 | 1.03:1 |
| TableManyItemsPerf.default | 2027 | 1960 | 1.03:1 |
| ButtonSlotsPerf.default | 570 | 557 | 1.02:1 |
| ChatMinimalPerf.default | 665 | 652 | 1.02:1 |
| CheckboxMinimalPerf.default | 2736 | 2692 | 1.02:1 |
| DatepickerMinimalPerf.default | 5375 | 5290 | 1.02:1 |
| DropdownMinimalPerf.default | 3050 | 2999 | 1.02:1 |
| FlexMinimalPerf.default | 285 | 280 | 1.02:1 |
| HeaderSlotsPerf.default | 799 | 785 | 1.02:1 |
| ListMinimalPerf.default | 538 | 529 | 1.02:1 |
| MenuMinimalPerf.default | 862 | 841 | 1.02:1 |
| TextMinimalPerf.default | 361 | 354 | 1.02:1 |
| DialogMinimalPerf.default | 766 | 759 | 1.01:1 |
| LoaderMinimalPerf.default | 681 | 673 | 1.01:1 |
| PortalMinimalPerf.default | 168 | 167 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1547 | 1537 | 1.01:1 |
| CustomToolbarPrototype.default | 3766 | 3746 | 1.01:1 |
| ButtonMinimalPerf.default | 177 | 177 | 1:1 |
| CarouselMinimalPerf.default | 466 | 466 | 1:1 |
| EmbedMinimalPerf.default | 4097 | 4107 | 1:1 |
| FormMinimalPerf.default | 432 | 431 | 1:1 |
| ListNestedPerf.default | 576 | 575 | 1:1 |
| PopupMinimalPerf.default | 595 | 594 | 1:1 |
| SplitButtonMinimalPerf.default | 3884 | 3886 | 1:1 |
| TableMinimalPerf.default | 399 | 399 | 1:1 |
| TooltipMinimalPerf.default | 1022 | 1021 | 1:1 |
| TreeMinimalPerf.default | 813 | 817 | 1:1 |
| DropdownManyItemsPerf.default | 687 | 696 | 0.99:1 |
| InputMinimalPerf.default | 1232 | 1248 | 0.99:1 |
| ProviderMinimalPerf.default | 958 | 970 | 0.99:1 |
| ReactionMinimalPerf.default | 390 | 392 | 0.99:1 |
| RefMinimalPerf.default | 229 | 231 | 0.99:1 |
| StatusMinimalPerf.default | 702 | 708 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1660 | 1688 | 0.98:1 |
| ListCommonPerf.default | 640 | 656 | 0.98:1 |
| RosterPerf.default | 1236 | 1256 | 0.98:1 |
| SliderMinimalPerf.default | 1519 | 1546 | 0.98:1 |
| ToolbarMinimalPerf.default | 946 | 963 | 0.98:1 |
| AttachmentMinimalPerf.default | 157 | 163 | 0.96:1 |
| IconMinimalPerf.default | 599 | 624 | 0.96:1 |
| TreeWith60ListItems.default | 175 | 184 | 0.95:1 |
| AccordionMinimalPerf.default | 152 | 162 | 0.94:1 |
Co-authored-by: Martin Hochel <hochelmartin@gmail.com>
khmakoto
left a comment
khmakoto
left a comment
There was a problem hiding this comment.
These changes look great, thanks for doing this!
I left a couple of super minor comments and one or two questions that I would like answers for but in general I agree with this 😄
|
|
||
| #### appearence of stories | ||
|
|
||
| Public stories should follow Fluent Design Language to give developers better feel for patterns they should utilise. For example, when a button is necessary to demonstrate usage of a component, Fluent UI Button should be used instead of a pure HTML button. |
There was a problem hiding this comment.
nit:
| Public stories should follow Fluent Design Language to give developers better feel for patterns they should utilise. For example, when a button is necessary to demonstrate usage of a component, Fluent UI Button should be used instead of a pure HTML button. | |
| Public stories should follow Fluent Design Language to give developers better feel for patterns they should utilize. For example, when a button is necessary to demonstrate usage of a component, Fluent UI Button should be used instead of a pure HTML button. |
There was a problem hiding this comment.
Is this feasible without introducing circular dependencies?
There was a problem hiding this comment.
Probably not, but I think this kind of circular dependencies are not an issue. These dependencies don’t need to be specified in package.json. The individual packages stay independent.
There was a problem hiding this comment.
Is this feasible without introducing circular dependencies?
@khmakoto there are no real circular dependencies -> that is solved with the new DX for vNext
context/refresher:
more details https://github.com/microsoft/fluentui/blob/master/rfcs/build-system/01_convergence_devstack.md
- packages must specify
dependenciesonly for packages that are being used for runtime code, not for stories code.
// @filename component.tsx
import {Button} from '@fui/button'
export const Component = (props) => { <div><Button>click</Button></div> };
---
// @filename component.stories.tsx
import {Button} from '@fui/button'
import {Menu} from '@fui/menu'
export const Example = (args) => { <Menu><Button>click</Button></Menu> };
// @filename package.json
{
"name": "@fui/component",
"dependencies": {
"@fui/button": "9.0.0-alpha.123"
// NO @fui/menu specified as that dependency is not being used in implementation code
}
}
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
| ### 8. dissecting big story files into smaller ones | ||
| 1. Every component can have at most one `.stories.tsx` file with default export which configures metadata about the component. This file must be called `Component.stories.tsx`, for example `Button.stories.tsx`. | ||
| 2. If putting all stories into one file would make it too long, individual stories might be put into additional `.stories.tsx` files as a named export, and then re-exported from `Component.stories.tsx` file like this: `export * from ‘./IndividualStoryFile.stories’;` | ||
| 3. If individual story files are employed, `Component.stories.tsx` file must not contain any stories. |
There was a problem hiding this comment.
the file that will be rendered must contain a story - the default export` which is triggerpoint that it should be rendered in tree view. can we mention that here explicitly -> "besides having the default export ...."
- text is great but if we could provide examples it would be more easier to understand 🙏
|
|
||
| ```tsx | ||
| export const ButtonPrimary = (props: ButtonProps) => <Button {...props}>Text</Button>; | ||
| ButtonPrimary.args = { |
There was a problem hiding this comment.
nope I meant args, args is imperative way how to set component prop values
|
|
||
| ### 9. UX of stories | ||
|
|
||
| #### first story is called default |
There was a problem hiding this comment.
| #### first story is called default | |
| #### main component story | |
| - is named `Default` -> `export const Default = () => {...}` |
There was a problem hiding this comment.
What do you think about "default story"? I think it’s better to keep naming consistent
There was a problem hiding this comment.
TBH not sure :D , after our discussion yesterday it would make sense to name it Playground or ApiPlayground which will express the intent in better way than Default
|
|
||
| #### first story is called default | ||
|
|
||
| The first story in every component must be called `Default`. This story must support manipulation with [Controls](https://storybook.js.org/docs/react/essentials/controls). More details in [chapter 3](#3-should-controls-work-for-all-stories-or-only-for-generaldefault-one). |
There was a problem hiding this comment.
The first story in every component must be called Default.
can we write the rules as bullet points ?
- again it's easier to read.
- I'd reword the 1st sentence a bit
↓↓↓
The main story for every component must adhere to following rules
- named as `Default`
- will be rendered as 1st story in tree view
- supports auto generated controls
- ...etc
|
|
||
| Button.stories.tsx: | ||
| ```tsx | ||
| import { Meta } from '@storybook/react'; |
There was a problem hiding this comment.
suggestion no need to add those as we need to properly document in another RFC way how we wanna type stories
There was a problem hiding this comment.
How about we keep it for now and remove it in the PR that’ll propose typing of stories? Just a safer way in my opinion, cause plans don’t always get done 😆
This is just proper usage of Storybook with Typescript according to their docs, so no new best practice/convention is being proposed here - it’s just a reminder.
There was a problem hiding this comment.
How about we keep it for now and remove it in the PR that’ll propose typing of stories?
thats definitely works. please not that this comment type was a suggestion -> pls refer to following standard to understand type of comments in the future https://conventionalcomments.org/
7 participants
Description of changes
Proposal for:
Deals with some of the same issues as #18895