-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
UI: Improve controls addon #23635
Changes from all commits
ea1b224
cc19cff
fd67d96
b6cdb0c
997abea
2f9ce19
81d6bd9
4a717d9
3ed5f7a
89ecd7e
75935b9
d156fb9
dbd77c8
a3a6327
22cfba6
716064c
f524837
e5734eb
90eb841
ce53df8
681cd17
1b6eb11
04ab1d2
1423bed
6ab4dea
c5f6677
1b138a9
e206d21
fa6d316
df56d8b
f3d2828
0ed119c
8a5ce35
b22fd14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||||||||||||||
|
@@ -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); | ||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm honestly not sure what happens if you call
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||||
|
@@ -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 |
---|---|---|
@@ -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, | ||
|
@@ -18,6 +19,7 @@ export interface ArgControlProps { | |
row: ArgType; | ||
arg: any; | ||
updateArgs: (args: Args) => void; | ||
isHovered: boolean; | ||
} | ||
|
||
const Controls: Record<string, FC> = { | ||
|
@@ -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); | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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.
Minor suggestion here:
It requires that you move this below
useStorybookState()
.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.
Thanks. I made the change.