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.

Sign up for GitHub

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

♻️Migrate Stories 62..65 to args #37730

Merged
merged 6 commits into from
Mar 18, 2022
Merged

♻️Migrate Stories 62..65 to args #37730

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf79d74
Remove knobs and add args to amp-layout, context, wrappers.
rbeckthomas Feb 18, 2022
34ed769
Merge branch 'main' of github.com:ampproject/amphtml into remove-knob…
rbeckthomas Mar 15, 2022
c372738
Move all inferrable type controls from argTypes to args.
rbeckthomas Mar 15, 2022
1149b57
Return accidentally removed 'withA11y' error.
rbeckthomas Mar 15, 2022
647d267
Re-order attributes to include {...args} first because subsequent values
rbeckthomas Mar 16, 2022
e16cbad
Add default value for wrapperStyle.
rbeckthomas Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions build-system/tasks/storybook/env/amp/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
const {globExcludeDisabledStorybookFiles} = require('../disabled-stories');
const {getStaticDirs} = require('../static-dirs');

const rootDir = '../../../../..';

module.exports = {
staticDirs: getStaticDirs(rootDir),
stories: globExcludeDisabledStorybookFiles([
stories: [
`${rootDir}/src/builtins/storybook/*.amp.js`,
`${rootDir}/extensions/**/*.*/storybook/*.amp.js`,
]),
],
addons: [
// TODO(alanorozco): AMP previews are loaded inside an iframe, so the a11y
// addon is not able to inspect the tree inside it. Its results are incorrect,
Expand Down
46 changes: 0 additions & 46 deletions build-system/tasks/storybook/env/disabled-stories.js
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions build-system/tasks/storybook/env/preact/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
const {globExcludeDisabledStorybookFiles} = require('../disabled-stories');
const {getStaticDirs} = require('../static-dirs');

const rootDir = '../../../../..';

module.exports = {
staticDirs: getStaticDirs(rootDir),
stories: globExcludeDisabledStorybookFiles([
stories: [
`${rootDir}/src/**/storybook/!(*.amp).js`,
`${rootDir}/extensions/**/*.*/storybook/!(*.amp).js`,
]),
],
addons: [
'@storybook/addon-a11y',
'@storybook/addon-viewport/register',
Expand Down
5 changes: 2 additions & 3 deletions build-system/tasks/storybook/env/react/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const {globExcludeDisabledStorybookFiles} = require('../disabled-stories');
const {getStaticDirs} = require('../static-dirs');

const rootDir = '../../../../..';
Expand All @@ -8,10 +7,10 @@ module.exports = {
// Unlike the `amp` and `preact` environments, we search Storybook files only
// under component paths. This is because only components have React build
// output, but directories in src/ outside src/bento/components/ do not.
stories: globExcludeDisabledStorybookFiles([
stories: [
`${rootDir}/extensions/**/*.*/storybook/!(*.amp).js`,
`${rootDir}/src/bento/components/**/*.*/storybook/*.js`,
]),
],
addons: [
'@storybook/addon-a11y',
'@storybook/addon-viewport/register',
Expand Down
10 changes: 0 additions & 10 deletions build-system/test-configs/forbidden-terms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,16 +720,6 @@ const forbiddenTermsGlobal = {
},
'withA11y':
'The Storybook decorator "withA11y" has been deprecated. You may simply remove it, since the a11y addon is now globally configured.',
'@storybook/addon-knobs': {
message:
'The @storybook/addon-knobs package has been deprecated. Use Controls instead (`args` and `argTypes`). https://storybook.js.org/docs/react/essentials/controls',
allowlist: [
// TODO(#35923): Update existing files to use Controls instead.
'src/builtins/storybook/amp-layout.amp.js',
'src/preact/storybook/Context.js',
'src/preact/storybook/Wrappers.js',
],
},
};

const bannedTermsHelpString =
Expand Down
23 changes: 14 additions & 9 deletions src/builtins/storybook/amp-layout.amp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
import {withAmp} from '@ampproject/storybook-addon';
import {number, withKnobs} from '@storybook/addon-knobs';

import * as Preact from '#preact';

export default {
title: '0/amp-layout',
decorators: [withKnobs, withAmp],
decorators: [withAmp],
};

export const responsive = () => {
const width = number('width', 400);
const height = number('height', 300);
export const responsive = ({height, width}) => {
return (
<main>
<style jsx global>
Expand All @@ -34,10 +31,12 @@ export const responsive = () => {
);
};

export const intrinsic = () => {
const width = number('width', 800);
const height = number('height', 600);
const maxWidth = number('maxWidth', 400);
responsive.args = {
width: 400,
height: 300,
};

export const intrinsic = ({height, maxWidth, width}) => {
return (
<main>
<style jsx global>
Expand Down Expand Up @@ -69,3 +68,9 @@ export const intrinsic = () => {
</main>
);
};

intrinsic.args = {
width: 800,
height: 600,
maxWidth: 400,
};
31 changes: 16 additions & 15 deletions src/preact/storybook/Context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,37 @@
// @ts-ignore
import {boolean, select, withKnobs} from '@storybook/addon-knobs';

import {Loading_Enum} from '#core/constants/loading-instructions';

import * as Preact from '#preact';
import {WithAmpContext, useAmpContext, useLoading} from '#preact/context';

export default {
title: '0/Context',
decorators: [withKnobs],
};

const IMG_SRC =
"data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' width='24px' height='24px' viewBox='0 0 24 24' fill='%23000000'%3E%3Cpath d='M0 0h24v24H0z' fill='none'/%3E%3Cpath d='M9 16.17L4.83 12l-1.42 1.41L9 19 21 7l-1.41-1.41z'/%3E%3C/svg%3E";

const LOADING_OPTIONS = ['auto', 'lazy', 'eager', 'unload'];

export const _default = () => {
Copy link
Member

Choose a reason for hiding this comment

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

FMI: why is this named _default? Does that do something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a convention we name the first story _default to make it clear its one that gets default selected when you navigate to the story. It doesnt do anything special in the storybook its more just for us

the default on line 9/6 That one does some storybook internal magic and is important to be named that way

const renderable = boolean('top renderable', true);
const playable = boolean('top playable', true);
const loading = select('top loading', LOADING_OPTIONS, LOADING_OPTIONS[0]);
export const _default = ({...args}) => {
return (
<WithAmpContext
renderable={renderable}
playable={playable}
loading={loading}
>
<WithAmpContext {...args}>
<Composite />
</WithAmpContext>
);
};

_default.args = {
renderable: true,
playable: true,
};

_default.argTypes = {
loading: {
name: 'loading',
defaultValue: 'auto',
options: ['auto', 'lazy', 'eager', 'unload'],
control: {type: 'select'},
},
};

/**
* @return {import('preact').VNode}
*/
Expand Down
91 changes: 52 additions & 39 deletions src/preact/storybook/Wrappers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,75 @@
// @ts-ignore
import {boolean, object, text, withKnobs} from '@storybook/addon-knobs';

// @ts-nocheck
import * as Preact from '#preact';
import {ContainWrapper, Wrapper} from '#preact/component';

export default {
title: '0/Wrappers',
decorators: [withKnobs],
};

export const wrapper = () => {
const asProp = text('As', 'span');
const className = text('className', '');
const style = object('style', {border: '1px solid'});
const wrapperClassName = text('wrapperClassName', '');
const wrapperStyle = object('wrapperStyle', {});
const ariaLabel = text('ariaLabel', 'aria label');
export const wrapper = ({ariaLabel, asProp, className, ...args}) => {
return (
<Wrapper
as={asProp}
class={className}
style={style}
wrapperClassName={wrapperClassName}
wrapperStyle={wrapperStyle}
aria-label={ariaLabel}
>
<Wrapper as={asProp} class={className} aria-label={ariaLabel} {...args}>
content
</Wrapper>
);
};

export const containWrapper = () => {
const asProp = text('As', 'div');
const className = text('className', '');
const style = object('style', {border: '1px solid', width: 200, height: 50});
const wrapperClassName = text('wrapperClassName', '');
const wrapperStyle = object('wrapperStyle', {padding: 4});
const contentClassName = text('contentClassName', 'content');
const contentStyle = object('contentStyle', {border: '1px dotted'});
const size = boolean('size', true);
const layout = boolean('layout', true);
const paint = boolean('paint', true);
const ariaLabel = text('ariaLabel', 'aria label');
wrapper.args = {
asProp: 'span',
ariaLabel: 'aria Label',
className: '',
wrapperClassName: '',
};

wrapper.argTypes = {
style: {
name: 'style',
defaultValue: {border: '1px solid'},
control: {type: 'object'},
},
wrapperStyle: {
name: 'wrapperStyle',
control: {type: 'object'},
},
};

export const containWrapper = ({ariaLabel, asProp, className, ...args}) => {
return (
<ContainWrapper
as={asProp}
class={className}
style={style}
wrapperClassName={wrapperClassName}
wrapperStyle={wrapperStyle}
contentClassName={contentClassName}
contentStyle={contentStyle}
size={size}
layout={layout}
paint={paint}
aria-label={ariaLabel}
{...args}
Copy link
Contributor

Choose a reason for hiding this comment

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

args contains some of the same properties that are passed in. Perhaps you want to do:

<ContainWrapper {...args} as={asProp} class={className} ...

>
content
</ContainWrapper>
);
};

containWrapper.args = {
asProp: 'div',
ariaLabel: 'aria Label',
className: '',
contentClassName: 'content',
wrapperClassName: '',
size: true,
layout: true,
paint: true,
};

containWrapper.argTypes = {
style: {
name: 'style',
defaultValue: {border: '1px solid', width: 200, height: 50},
control: {type: 'object'},
},
wrapperStyle: {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need the default value of {padding: 4}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes! thank you :)

name: 'wrapperStyle',
control: {type: 'object'},
},
contentStyle: {
name: 'contentStyle',
defaultValue: {border: '1px dotted'},
control: {type: 'object'},
},
};