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(rfc): create storybook authoring proposal #18455

Merged

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented Jun 4, 2021

Pull request checklist

  • [ ] Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

  • adds initial style guide proposal for authoring stories for convergence. READ IT IN PREVIEW MODE
  • DEMO: improves controls for all stories in react-popover

Focus areas to test

(optional)

@size-auditor
Copy link

size-auditor bot commented Jun 4, 2021

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: fd00f462735d6ea774d28c581079d5ad9e7e3945 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 4, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 819 815 5000
BaseButton mount 888 895 5000
Breadcrumb mount 2696 2626 1000
ButtonNext mount 518 538 5000
Checkbox mount 1536 1509 5000
CheckboxBase mount 1312 1267 5000
ChoiceGroup mount 4702 4753 5000
ComboBox mount 984 964 1000
CommandBar mount 10331 10338 1000
ContextualMenu mount 6329 6291 1000
DefaultButton mount 1132 1110 5000
DetailsRow mount 3689 3731 5000
DetailsRowFast mount 3755 3822 5000
DetailsRowNoStyles mount 3563 3545 5000
Dialog mount 2148 2158 1000
DocumentCardTitle mount 152 133 1000
Dropdown mount 3201 3285 5000
FluentProviderNext mount 7325 7318 5000
FocusTrapZone mount 1782 1744 5000
FocusZone mount 1819 1842 5000
IconButton mount 1772 1706 5000
Label mount 333 326 5000
Layer mount 1790 1788 5000
Link mount 466 477 5000
MakeStyles mount 1833 1864 50000
MenuButton mount 1475 1488 5000
MessageBar mount 2051 2044 5000
Nav mount 3226 3230 1000
OverflowSet mount 1019 1060 5000
Panel mount 2115 2155 1000
Persona mount 798 851 1000
Pivot mount 1419 1421 1000
PrimaryButton mount 1276 1282 5000
Rating mount 7629 7550 5000
SearchBox mount 1326 1337 5000
Shimmer mount 2493 2540 5000
Slider mount 1932 1948 5000
SpinButton mount 4982 4966 5000
Spinner mount 423 425 5000
SplitButton mount 3144 3113 5000
Stack mount 513 516 5000
StackWithIntrinsicChildren mount 1483 1503 5000
StackWithTextChildren mount 4493 4464 5000
SwatchColorPicker mount 10140 10178 5000
Tabs mount 1387 1444 1000
TagPicker mount 2406 2430 5000
TeachingBubble mount 11986 11981 5000
Text mount 423 414 5000
TextField mount 1358 1373 5000
ThemeProvider mount 1239 1196 5000
ThemeProvider virtual-rerender 605 619 5000
Toggle mount 801 819 5000
buttonNative mount 107 118 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 173 154 1.12:1
AvatarMinimalPerf.default 203 189 1.07:1
DividerMinimalPerf.default 368 343 1.07:1
PortalMinimalPerf.default 189 177 1.07:1
VideoMinimalPerf.default 634 605 1.05:1
HeaderMinimalPerf.default 361 348 1.04:1
ListMinimalPerf.default 524 503 1.04:1
AttachmentSlotsPerf.default 1097 1060 1.03:1
ButtonOverridesMissPerf.default 1707 1656 1.03:1
InputMinimalPerf.default 1277 1239 1.03:1
LabelMinimalPerf.default 389 379 1.03:1
ListNestedPerf.default 551 534 1.03:1
SplitButtonMinimalPerf.default 3762 3649 1.03:1
TableMinimalPerf.default 415 403 1.03:1
FormMinimalPerf.default 399 393 1.02:1
LoaderMinimalPerf.default 708 697 1.02:1
MenuMinimalPerf.default 848 828 1.02:1
PopupMinimalPerf.default 588 574 1.02:1
ReactionMinimalPerf.default 369 360 1.02:1
SkeletonMinimalPerf.default 355 348 1.02:1
SliderMinimalPerf.default 1574 1544 1.02:1
TableManyItemsPerf.default 1928 1891 1.02:1
AnimationMinimalPerf.default 413 407 1.01:1
CardMinimalPerf.default 557 549 1.01:1
ChatMinimalPerf.default 644 640 1.01:1
DropdownManyItemsPerf.default 676 670 1.01:1
FlexMinimalPerf.default 293 290 1.01:1
HeaderSlotsPerf.default 763 757 1.01:1
ImageMinimalPerf.default 369 364 1.01:1
ItemLayoutMinimalPerf.default 1237 1220 1.01:1
ListCommonPerf.default 633 628 1.01:1
ListWith60ListItems.default 653 648 1.01:1
MenuButtonMinimalPerf.default 1573 1550 1.01:1
ProviderMergeThemesPerf.default 1681 1665 1.01:1
TextAreaMinimalPerf.default 496 493 1.01:1
CustomToolbarPrototype.default 3827 3800 1.01:1
TreeMinimalPerf.default 803 795 1.01:1
AlertMinimalPerf.default 274 275 1:1
BoxMinimalPerf.default 357 356 1:1
ButtonMinimalPerf.default 160 160 1:1
ChatWithPopoverPerf.default 360 359 1:1
CheckboxMinimalPerf.default 2777 2777 1:1
DialogMinimalPerf.default 749 750 1:1
DropdownMinimalPerf.default 3088 3079 1:1
GridMinimalPerf.default 344 343 1:1
ProviderMinimalPerf.default 975 978 1:1
RefMinimalPerf.default 237 238 1:1
SegmentMinimalPerf.default 348 348 1:1
ButtonSlotsPerf.default 543 548 0.99:1
DatepickerMinimalPerf.default 5454 5490 0.99:1
EmbedMinimalPerf.default 4120 4152 0.99:1
StatusMinimalPerf.default 666 676 0.99:1
TooltipMinimalPerf.default 967 976 0.99:1
TreeWith60ListItems.default 174 175 0.99:1
RadioGroupMinimalPerf.default 435 443 0.98:1
IconMinimalPerf.default 595 606 0.98:1
TextMinimalPerf.default 340 347 0.98:1
ToolbarMinimalPerf.default 916 932 0.98:1
ChatDuplicateMessagesPerf.default 282 292 0.97:1
RosterPerf.default 1128 1163 0.97:1
AccordionMinimalPerf.default 141 147 0.96:1
CarouselMinimalPerf.default 448 466 0.96:1
LayoutMinimalPerf.default 359 376 0.95:1

@Hotell Hotell changed the title WIP: chore(react-popover): make controlls work for all stories - better DX docs(rfc): create storybook authoring proposal Jun 8, 2021
@Hotell Hotell added the Type: RFC Request for Feedback label Jun 8, 2021
@Hotell Hotell marked this pull request as ready for review June 8, 2021 10:32
Copy link
Contributor

@andrefcdias andrefcdias left a comment

Choose a reason for hiding this comment

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

Just some suggestions but the approach is good.

My personal opinion on 3. is that we should go with the provide controls with control pane only for default/playground story option, I'm not able to see what the benefit would be for the other solution.

@Hotell Hotell requested review from a team June 8, 2021 16:38
@miroslavstastny
Copy link
Member

Can we also discuss the stories structure in the nav tree as it might be related?

Current problems

  • users are confused by Canvas vs Docs tabs is the Storybook
  • existing stories lack any documentation
  • it is not clear which stories are meant to be user docs vs inner dev loop (and e2e) stories

Proposed solution

  • always hide the Canvas/Docs switch
  • structure the stories as following:
Popover
  OVERVIEW
  Anchor To Target
  Controlled
  _Internal
    Dev story 1
    E2E story 1    

OVERVIEW is always rendered as 'Docs', it contains following parts:

  1. Textual docs for the component and Best practices
  2. Component playground with controls
  3. Args table (this might overlap with playground controls, but is not necessarily the same)
  4. All "public" stories (Anchor To Target and Controlled in the example above)
    • textual documentation
    • example without any controls
    • possibility to show meaningful source code

After OVERVIEW in the tree, there are Anchor To Target and Controlled - public stories, always rendered as 'Canvas'.

Last in the tree, there is a directory _Internal which contains all the stories which are not meant to be public but used for inner dev loop - dev stories, e2e stories...
We might eventually decide to omit these internal stories in the public build of the docs.

@ling1726
Copy link
Member

ling1726 commented Jun 9, 2021

Should we also point out that if the main .stories.tsx file is getting large, then the authors should consider splitting up stories among several files and simply linking them together:

// Every file with this export will appear under the same storybook category/component
export default {
  title: 'Components/Component',
  component: Component,
};

@Hotell
Copy link
Contributor Author

Hotell commented Jun 9, 2021

Should we also point out that if the main .stories.tsx file is getting large, then the authors should consider splitting up stories among several files and simply linking them together:

// Every file with this export will appear under the same storybook category/component
export default {
  title: 'Components/Component',
  component: Component,
};

totally agree but let's do it as followup. WDYT ?

@Hotell
Copy link
Contributor Author

Hotell commented Jun 9, 2021

Can we also discuss the stories structure in the nav tree as it might be related?

@miroslavstastny yes please! let's do that as follow up. As mentioned in the document this is just an initial kickoff for discussion/decisions and it should be a living document and a main source of truth for how we wanna author stories -> when that's done we can create nx generators to autogenerate stories as we please etc... thx

@Hotell
Copy link
Contributor Author

Hotell commented Jun 10, 2021

Update:

  • added sections for follow ups
  • marked proposal to use controls only in playground only as preferred and wrapped the 2nd on within details for less distraction for the reader
    • 2021-06-10 at 10 34

@Hotell Hotell force-pushed the hotell/storybook-controls-improvements-popover branch 2 times, most recently from f4d24c4 to 66bfa6c Compare June 10, 2021 10:13
@Hotell Hotell force-pushed the hotell/storybook-controls-improvements-popover branch from 66bfa6c to 1959b28 Compare June 10, 2021 11:01
@Hotell Hotell requested a review from a team as a code owner June 10, 2021 11:01

## Summary

Currently we have no style-guide/functionality requirements on how to be consistent when writing stories for converged components. This RFC should describe (as we progress) the style we want going forward.
Copy link
Contributor

Choose a reason for hiding this comment

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

For a little more background/context: is the intent that this storybook is the primary documentation for our controls? It might be worthwhile to mention that here, as motivation to why we want to have consistency in the storybook. And if so, it would be great to eventually make a permanent documentation authoring guide based on the decisions in this RFC.

Copy link
Member

@ling1726 ling1726 Jun 11, 2021

Choose a reason for hiding this comment

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

is the intent that this storybook is the primary documentation for our controls?

I have some historical knowledge here. The storybook is not intended to be our docs, but currently they are the only way we have of communicating any sort of docs with our partners AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the intent is to have consistency and guidelines ideally for everything (not just storybook stories) for more effective collaboration and automation capabilities :)

regarding official docs, that is still in flux, partially what @ling1726 said. Hopefully we will leverage as much as possible from stories for final docs solution.


> This is preferred approach - Based on feedback from @teams-prg/@cxe-prg team

With that approach we would probably need to completely get rid of `controls` addon pane from all stories except `Docs` view and our `Default/Playground` story.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd propose that the default story should not have any controls, and instead have some preset examples showing basic usage of the control. Only the playground story(ies) should have controls -- they're useful if you want to try out more of the functionality of the component, but it's better to have some functional examples to start with on the default story, for someone learning a new control library.

Copy link
Member

Choose a reason for hiding this comment

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

I think the Default/Plaground story should be one and the same thing. We need the simplest variation of this component to let the user test as many props as possible.


With that approach we would probably need to completely get rid of `controls` addon pane from all stories except `Docs` view and our `Default/Playground` story.

This can be achieved by following config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Once this RFC is merged, could you follow up and update the template for the create-component script, to include any additional config that's required?
https://github.com/microsoft/fluentui/tree/master/scripts/create-component/plop-templates-storybook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm not sure what parts are applicable TBH.

the current workflow is following:

in future we'll migrate those existing generators to nx generators, which will also enable us to generate stories based on implementation of components automatically (Playground/Default)

@behowell
Copy link
Contributor

Should we also point out that if the main .stories.tsx file is getting large, then the authors should consider splitting up stories among several files and simply linking them together:

@ling1726 @Hotell Perhaps we should make it the default to have every story in its own file? Rather than only doing that once the file gets large... it could just be better organized from the start.

@ling1726
Copy link
Member

Should we also point out that if the main .stories.tsx file is getting large, then the authors should consider splitting up stories among several files and simply linking them together:

@ling1726 @Hotell Perhaps we should make it the default to have every story in its own file? Rather than only doing that once the file gets large... it could just be better organized from the start.

@behowell Our current build process does not allow splitting files for stories of the same component. @Hotell 's work should enable this soon.

I think there is value to split things. I don't know if we should religiously stick to one story per file. TBH I don't mind, I just want to be able to at least split longer more complicated stories out.

@Hotell
Copy link
Contributor Author

Hotell commented Jun 14, 2021

Should we also point out that if the main .stories.tsx file is getting large, then the authors should consider splitting up stories among several files and simply linking them together:

@ling1726 @Hotell Perhaps we should make it the default to have every story in its own file? Rather than only doing that once the file gets large... it could just be better organized from the start.

@behowell Our current build process does not allow splitting files for stories of the same component. @Hotell 's work should enable this soon.

I think there is value to split things. I don't know if we should religiously stick to one story per file. TBH I don't mind, I just want to be able to at least split longer more complicated stories out.

One story per file might be an unnecessary granularity from my experience. Whilst I don't mind I'd recommend to rather focus on for which components should we write those stories

  • storyFile:componentFile -> 1:1
  • or as we currently have -> only higher level stories that use multiple components - storyFile:componentFiles -> 1:N

Another solution that we might investigate later is following https://storybook.js.org/docs/react/workflows/stories-for-multiple-components (but based on current "decision" we wanna leverage controls only for Default/Playground)

@ling1726
Copy link
Member

ling1726 commented Jul 7, 2021

@Hotell can we get this merged since it quite a bit of approvals ? I would like to add a proposal about internal storybook stories to this RFC

@Hotell Hotell force-pushed the hotell/storybook-controls-improvements-popover branch from 1959b28 to 7604200 Compare July 8, 2021 08:22
@Hotell Hotell enabled auto-merge (squash) July 8, 2021 08:23
@Hotell
Copy link
Contributor Author

Hotell commented Jul 8, 2021

@Hotell can we get this merged since it quite a bit of approvals ? I would like to add a proposal about internal storybook stories to this RFC

sorry it took so long :) . once pipeline passes it will be merged. thanks !

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 8, 2021

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 4bcbf22:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@Hotell Hotell force-pushed the hotell/storybook-controls-improvements-popover branch from 7604200 to 4bcbf22 Compare July 8, 2021 09:27
@Hotell Hotell merged commit a01de06 into microsoft:master Jul 8, 2021
PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
* docs(rfc): create storybook authoring proposal
* docs(rfc): apply suggestions from code review
* docs(rfc): add next sections to cover in future PRs

Co-authored-by: André Dias <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: RFC Request for Feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants