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

UI: Improve controls addon #23635

Merged
merged 34 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ea1b224
Improve loading for controls
cdedreuille Jul 27, 2023
cc19cff
Merge branch 'next' into charles-update-controls-addon
cdedreuille Jul 28, 2023
fd67d96
Made loading works
cdedreuille Jul 31, 2023
b6cdb0c
Fix types error
cdedreuille Jul 31, 2023
997abea
Update ControlsPanel.tsx
cdedreuille Jul 31, 2023
2f9ce19
Add Link component
cdedreuille Jul 31, 2023
81d6bd9
Update
cdedreuille Jul 31, 2023
4a717d9
Fix links
cdedreuille Jul 31, 2023
3ed5f7a
Improve Link + Icons
cdedreuille Jul 31, 2023
89ecd7e
Icons are now working :)
cdedreuille Jul 31, 2023
75935b9
Improve Link component
cdedreuille Jul 31, 2023
d156fb9
Improve Link
cdedreuille Jul 31, 2023
dbd77c8
Merge branch 'next' into charles-update-controls-addon
cdedreuille Jul 31, 2023
a3a6327
Improve link
cdedreuille Jul 31, 2023
22cfba6
Update Empty.tsx
cdedreuille Jul 31, 2023
716064c
Remove FakeIcon
cdedreuille Jul 31, 2023
f524837
Add hover link when not set up
cdedreuille Aug 1, 2023
e5734eb
Update Link.tsx
cdedreuille Aug 1, 2023
90eb841
Fix some icons names
cdedreuille Aug 1, 2023
ce53df8
Update DocsPage.stories.tsx
cdedreuille Aug 1, 2023
681cd17
add icons to verdaccio proxy config
ndelangen Aug 1, 2023
1b6eb11
add proper alias for components/experimental
ndelangen Aug 1, 2023
04ab1d2
Remove NoControlWarning
cdedreuille Aug 1, 2023
1423bed
Merge branch 'charles-update-controls-addon' of https://github.com/st…
cdedreuille Aug 1, 2023
6ab4dea
Merge branch 'next' into charles-update-controls-addon
cdedreuille Aug 1, 2023
c5f6677
Test
cdedreuille Aug 1, 2023
1b138a9
Cleaning loading state
cdedreuille Aug 1, 2023
e206d21
Update ControlsPanel.tsx
cdedreuille Aug 1, 2023
fa6d316
Merge branch 'next' into charles-update-controls-addon
cdedreuille Aug 1, 2023
df56d8b
Merge branch 'next' into charles-update-controls-addon
cdedreuille Aug 1, 2023
f3d2828
Merge branch 'next' into charles-update-controls-addon
cdedreuille Aug 1, 2023
0ed119c
Merge branch 'next' into charles-update-controls-addon
cdedreuille Aug 2, 2023
8a5ce35
Fix the Link style
cdedreuille Aug 2, 2023
b22fd14
Add delay to empty state to prevent flickering
cdedreuille Aug 2, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions code/addons/controls/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
"@storybook/client-logger": "workspace:*",
"@storybook/components": "workspace:*",
"@storybook/core-common": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/manager-api": "workspace:*",
"@storybook/node-logger": "workspace:*",
"@storybook/preview-api": "workspace:*",
Expand Down
57 changes: 25 additions & 32 deletions code/addons/controls/src/ControlsPanel.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
import type { FC } from 'react';
import React from 'react';
import React, { useEffect, useState } from 'react';
import {
useArgs,
useGlobals,
useArgTypes,
useParameter,
useStorybookState,
} from '@storybook/manager-api';
import {
PureArgsTable as ArgsTable,
NoControlsWarning,
type PresetColor,
type SortType,
} from '@storybook/blocks';
import { PureArgsTable as ArgsTable, type PresetColor, type SortType } from '@storybook/blocks';

import type { ArgTypes } from '@storybook/types';
import { PARAM_KEY } from './constants';
Expand All @@ -21,24 +16,26 @@ interface ControlsParameters {
sort?: SortType;
expanded?: boolean;
presetColors?: PresetColor[];

/** @deprecated No longer used, will be removed in Storybook 8.0 */
hideNoControlsWarning?: boolean;
}

export const ControlsPanel: FC = () => {
const [isLoading, setIsLoading] = useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion here:

Suggested change
const [isLoading, setIsLoading] = useState(true);
const [isLoading, setIsLoading] = useState(!previewInitiated);

It requires that you move this below useStorybookState().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I made the change.

const [args, updateArgs, resetArgs] = useArgs();
const [globals] = useGlobals();
const rows = useArgTypes();
const isArgsStory = useParameter<boolean>('__isArgsStory', false);
const {
expanded,
sort,
presetColors,
hideNoControlsWarning = false,
} = useParameter<ControlsParameters>(PARAM_KEY, {});
const { path } = useStorybookState();
const { expanded, sort, presetColors } = useParameter<ControlsParameters>(PARAM_KEY, {});
const { path, previewInitialized } = useStorybookState();

// If the story is prepared, then show the args table
// and reset the loading states
useEffect(() => {
if (previewInitialized) setIsLoading(false);
}, [previewInitialized]);
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm honestly not sure what happens if you call setState with the same state that it already has.

Suggested change
useEffect(() => {
if (previewInitialized) setIsLoading(false);
}, [previewInitialized]);
useEffect(() => {
if (previewInitialized && isLoading) setIsLoading(false);
}, [previewInitialized]);

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 agreed let's not do that as we are at risk of creating an infinite loop in the useEffect.

Copy link
Member

Choose a reason for hiding this comment

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

make sure to add isLoading and setIsLoading to the hook dependencies array too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We agreed not to do that @yannbf. It's working without it.


const hasControls = Object.values(rows).some((arg) => arg?.control);
const showWarning = !(hasControls && isArgsStory) && !hideNoControlsWarning;

const withPresetColors = Object.entries(rows).reduce((acc, [key, arg]) => {
if (arg?.control?.type !== 'color' || arg?.control?.presetColors) acc[key] = arg;
Expand All @@ -47,21 +44,17 @@ export const ControlsPanel: FC = () => {
}, {} as ArgTypes);

return (
<>
{showWarning && <NoControlsWarning />}
<ArgsTable
{...{
key: path, // resets state when switching stories
compact: !expanded && hasControls,
rows: withPresetColors,
args,
globals,
updateArgs,
resetArgs,
inAddonPanel: true,
sort,
}}
/>
</>
<ArgsTable
key={path} // resets state when switching stories
compact={!expanded && hasControls}
rows={withPresetColors}
args={args}
globals={globals}
updateArgs={updateArgs}
resetArgs={resetArgs}
inAddonPanel
sort={sort}
isLoading={isLoading}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ const getAbsolutePath = <I extends string>(input: I): I =>
dirname(require.resolve(join(input, 'package.json'))) as any;

const storybookPaths: Record<string, string> = {
// this is a temporary hack to get webpack to alias this correctly
[`@storybook/components/experimental`]: `${getAbsolutePath(
`@storybook/components`
)}/dist/experimental`,
...[
// these packages are not pre-bundled because of react dependencies
'components',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ import { DocInjectableService } from './doc-injectable.service';

export default {
component: DocInjectableService,
parameters: {
controls: { hideNoControlsWarning: true },
},
};

const modules = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ import { DocPipe } from './doc-pipe.pipe';

export default {
component: DocPipe,
parameters: {
controls: { hideNoControlsWarning: true },
},
};

const modules = {
Expand Down
1 change: 0 additions & 1 deletion code/ui/blocks/src/blocks/Description.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const meta: Meta<typeof Description> = {
parameters: {
controls: {
include: [],
hideNoControlsWarning: true,
},
// workaround for https://github.com/storybookjs/storybook/issues/20505
docs: { source: { type: 'code' } },
Expand Down
17 changes: 15 additions & 2 deletions code/ui/blocks/src/components/ArgsTable/ArgControl.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { FC } from 'react';
import React, { useCallback, useState, useEffect } from 'react';

import { Link } from '@storybook/components/experimental';
import {
BooleanControl,
ColorControl,
Expand All @@ -18,6 +19,7 @@ export interface ArgControlProps {
row: ArgType;
arg: any;
updateArgs: (args: Args) => void;
isHovered: boolean;
}

const Controls: Record<string, FC> = {
Expand All @@ -40,7 +42,7 @@ const Controls: Record<string, FC> = {

const NoControl = () => <>-</>;

export const ArgControl: FC<ArgControlProps> = ({ row, arg, updateArgs }) => {
export const ArgControl: FC<ArgControlProps> = ({ row, arg, updateArgs, isHovered }) => {
const { key, control } = row;

const [isFocused, setFocused] = useState(false);
Expand All @@ -63,7 +65,18 @@ export const ArgControl: FC<ArgControlProps> = ({ row, arg, updateArgs }) => {
const onBlur = useCallback(() => setFocused(false), []);
const onFocus = useCallback(() => setFocused(true), []);

if (!control || control.disable) return <NoControl />;
if (!control || control.disable)
return isHovered ? (
<Link
href="https://storybook.js.org/docs/react/essentials/controls"
target="_blank"
withArrow
>
Setup controls
</Link>
) : (
Comment on lines +68 to +77
Copy link

@MWhite-22 MWhite-22 Sep 19, 2023

Choose a reason for hiding this comment

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

@cdedreuille @JReinhold Can we find a way to make this Link optional? When rendering action props (aka: onChange, onClick, etc.) they have no control, but they're handled by the actions addon. With this change, even though their control should be empty, we render the link on hover.

I have other examples beyond just action props, but ideally I would like to globally disable any "Setup controls" links with a parameter or something similar.

Copy link

@MWhite-22 MWhite-22 Sep 19, 2023

Choose a reason for hiding this comment

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

For example, the only reason I need control: { type: null } on the below argTypes is to prevent the link from rendering on hover in the ArgsTable.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MWhite-22 Thanks for your feedback. I see what you mean but I'm not sure why this links is a problem. Are you using the controls as a tool or as a way to show what props you can edit?

Choose a reason for hiding this comment

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

We use storybook as public interactive docs for our component library.

The default behavior of rendering the link on hover makes it look like our docs are not completed, and we like to be incredibly thorough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a link I can look at? Generally speaking, I think there's a clear distinction between docs and controls. Docs are specifically meant to document your components where controls is more about testing. But I can understand that some users like you can use controls as a way to document your stories. I'll try to think if there's a way to achieve what you need without docs.

<NoControl />
);

// row.name is a display name and not a suitable DOM input id or name - i might contain whitespace etc.
// row.key is a hash key and therefore a much safer choice
Expand Down
7 changes: 4 additions & 3 deletions code/ui/blocks/src/components/ArgsTable/ArgRow.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { FC } from 'react';
import React from 'react';
import React, { useState } from 'react';
import Markdown from 'markdown-to-jsx';
import { transparentize } from 'polished';
import { styled } from '@storybook/theming';
Expand Down Expand Up @@ -76,6 +76,7 @@ const StyledTd = styled.td<{ expandable: boolean }>(({ theme, expandable }) => (
}));

export const ArgRow: FC<ArgRowProps> = (props) => {
const [isHovered, setIsHovered] = useState(false);
const { row, updateArgs, compact, expandable, initialExpandedArgs } = props;
const { name, description } = row;
const table = (row.table || {}) as TableAnnotation;
Expand All @@ -85,7 +86,7 @@ export const ArgRow: FC<ArgRowProps> = (props) => {
const hasDescription = description != null && description !== '';

return (
<tr>
<tr onMouseEnter={() => setIsHovered(true)} onMouseLeave={() => setIsHovered(false)}>
<StyledTd expandable={expandable}>
<Name>{name}</Name>
{required ? <Required title="Required">*</Required> : null}
Expand Down Expand Up @@ -118,7 +119,7 @@ export const ArgRow: FC<ArgRowProps> = (props) => {
)}
{updateArgs ? (
<td>
<ArgControl {...(props as ArgControlProps)} />
<ArgControl {...(props as ArgControlProps)} isHovered={isHovered} />
</td>
) : null}
</tr>
Expand Down
23 changes: 11 additions & 12 deletions code/ui/blocks/src/components/ArgsTable/ArgsTable.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import React from 'react';
import { action } from '@storybook/addon-actions';
import { styled } from '@storybook/theming';
import { ArgsTable, ArgsTableError } from './ArgsTable';
import { NoControlsWarning } from './NoControlsWarning';
import * as ArgRow from './ArgRow.stories';

export default {
Expand Down Expand Up @@ -58,15 +57,14 @@ export const InAddonPanel = {
decorators: [(storyFn: any) => <AddonPanelLayout>{storyFn()}</AddonPanelLayout>],
};

export const InAddonPanelWithWarning = {
render: (args: any) => (
<>
<NoControlsWarning />
<ArgsTable {...args} />
</>
),
// @ts-expect-error (not strict)
args: { ...InAddonPanel.args, updateArgs: null },
export const InAddonPanelNoControls = {
render: (args: any) => <ArgsTable {...args} />,
args: {
rows: {
stringType: { ...stringType, control: false },
numberType: { ...numberType, control: false },
},
},
decorators: InAddonPanel.decorators,
};

Expand Down Expand Up @@ -141,8 +139,9 @@ export const Error = {
};

export const Empty = {
args: {
rows: {},
args: {},
parameters: {
layout: 'centered',
},
};

Expand Down