-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Addon-docs: DocsPage and doc blocks #7119
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-7101-doc-blocks.storybook.now.sh |
@shilman can you ping me when this is ready to review (or else you could change the base temporarily) |
@tmeasday This is ready to review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review all of this but it seems generally high quality. Anything in particular you wanted me to look at?
I’m not so sure it will.. let’s try it?
…On 19 Jun 2019, 5:18 PM +1000, Michael Shilman ***@***.***>, wrote:
@shilman commented on this pull request.
In lib/components/src/blocks/PropsTable/PropsTable.stories.tsx:
> +import { PropsTable, PropsTableError } from './PropsTable';
+import { DocsPageWrapper } from '../DocsPage';
+import * as rowStories from './PropRow.stories';
+
+export default {
+ Component: PropsTable,
+ title: 'Docs|PropTable',
+ decorators: [storyFn => <DocsPageWrapper>{storyFn()}</DocsPageWrapper>],
+};
+
+export const error = () => <PropsTable error={PropsTableError.NO_COMPONENT} />;
+
+export const empty = () => <PropsTable rows={[]} />;
+
+const { row: stringRow } = rowStories.string().props;
+const { row: numberRow } = rowStories.number().props;
it works. i think exporting data will be cleaner tho!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
addons/docs/src/blocks/DocsPage.tsx
Outdated
expanded = true, | ||
}) => ( | ||
<> | ||
{expanded && <h3>{name}</h3>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be configurable at all?
why not just always show this?
Is creating a different docBlock for "showing all stories" not a better api?
configurability is the root of all evil
@@ -0,0 +1,136 @@ | |||
import React from 'react'; | |||
|
|||
import { parseKind } from '@storybook/router'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to rename this at some point, to something a little more descriptive of what is actually does.
|
||
const DocsPage: React.FunctionComponent<DocsPageProps> = ({ title, subtitle, children }) => ( | ||
<> | ||
{title && <Title>{title}</Title>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have shared typography components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should have them
@@ -0,0 +1,155 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse for markdown tables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set tables in a horizontal scroller container so mobile users can scroll to view the entire thing?
I think it currently makes up the entire viewport scrollable horizontally
<MyComponent boolProp scalarProp={1} complexProp={{ foo: 1, bar: '2' }}> | ||
<SomeOtherComponent funcProp={(a) => a.id} /> | ||
</MyComponent> | ||
`.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these trims are causing the bad indenting, because the formatter expects the whitespace from template literals.
})); | ||
|
||
export interface TypesetProps { | ||
fontSizes: number[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a font-face / font-family prop makes sense here?
Issue: #7101
This is based on Addon-docs: Basic skeleton, UI viewMode handling #7107 so wait until that's merged before reviewingAddon-docs: Module story format & framework param #7110and Addon-docs: Source loader library #7117 but can be reviewed without merging thoseWhat I did
This PR adds DocsPage and all the doc blocks. It's a big one, but hopefully the code is pretty simple-minded.
In summary:
lib/components/src/blocks/*
- pure components that have their own stories in official-storybookaddons/docs/src/blocks/*
- container components that do various tricks to get stuff out of the context/etc. to provide data to the pure componentsDocsPage
which automatically generates pages for each "kind"NOTES:
Source
block until Addon-docs: Source loader library #7117 source loading & "Docs preset" PR integrates the two.Props
block until Addon-docs: Module story format & framework param #7110 provides framework parameterHow to test