UI: Improve controls addon #23635

cdedreuille · 2023-07-27T14:47:31Z

What I did

In this PR I am improve how controls look when the story is loading and when the story doesn't have args set up. Today we are showing a yellow warning banner in both cases. This is not ideal and could be considered as a bug even though you sometimes don't need args. We are turning the narrative to make the message more descriptive of what controls are with clear links on how to get started.

Add a new Empty state when controls are not setup
Add a new Skeleton component when the story is loading
Added a link to setup controls on the controls column when there's no controls

This PR also includes:

Fix export for Select, Input in the experimental component library
Added a new Icon component (used in the empty state of the ArgsTable
Added a new Link component (Used in the Args Table)
Replace FakeIcon with the new Icon component
Fix experimental misspelling

How to test

Fetch this branch locally
Run Storybook UI locally using yarn storybook:ui
To test the loading state, access any story and reload the page
To test the empty state, go to http://localhost:6006/?path=/story/blocks-components-emptyblock--error&globals=theme:light
To test the "setup controls" link in the args table, go to https://635781f3500dd2c49e189caf-csejcfbxls.chromatic.com/?path=/story/components-scrollarea--vertical and hover any rows

Checklist

Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
Make sure to add/update documentation regarding your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

socket-security · 2023-07-31T14:43:56Z

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

socket-security · 2023-07-31T14:43:59Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

JReinhold

I'm experiencing some buggy behavior, as demonstrated here:

Area.mp4

On page load (reload), the panel is in a loading state. ✅
When switching to another addon panel and back to controls, the controls will be forever loading. ❌
When switching to new stories that have never been rendered before, it won't show the loading state but briefly flash the empty state. ❌
Navigating to previously rendered stories will show the controls right away without loading or empty-state ✅
Non-ready controls will have the "Setup controls" link on hover. ✅

Number 2 is a deal breaker here, but I think 3 is okay for now.

cdedreuille · 2023-08-02T06:35:59Z

Thanks @JReinhold. I'll look at these 2 use cases.

JReinhold · 2023-08-02T06:40:52Z

As I side note, I'm pretty sure there are some research on the topic of loading states, suggesting that there are ideal timings for when to show them or not.

Eg. in the first 0.3 seconds, a loading indicator is not necessary and will distract more than it does good, because of the brief flash.
It would be great to incorporate that into this design to give it the last polish.

Unfortunately I can't find said research, or figure out what this ideal timing is.

cdedreuille · 2023-08-02T06:43:16Z

@JReinhold That's not the issue. We don't want to show the loading state when switching stories as it is indeed too short but today if the story is cached the switch will be instantaneous which is nice but for stories not cached you see briefly the empty state instead. But I might have an idea on how to solve this one.

cdedreuille · 2023-08-02T08:16:55Z

Controls.mp4

@JReinhold I found a way to make 2 and 3 work so we never see the empty state if we don't need it. I added a delay to render the empty state. For now this is the only solution I believe without doing more refactoring or creating new states.

JReinhold

Minor feedback regarding your last changes

JReinhold · 2023-08-02T08:43:30Z

code/addons/controls/src/ControlsPanel.tsx

  hideNoControlsWarning?: boolean;
 }

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


Minor suggestion here:

Suggested change

const [isLoading, setIsLoading] = useState(true);

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

It requires that you move this below useStorybookState().

Thanks. I made the change.

JReinhold · 2023-08-02T08:44:50Z

code/addons/controls/src/ControlsPanel.tsx

+  useEffect(() => {
+    if (previewInitialized) setIsLoading(false);
+  }, [previewInitialized]);


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]);

As agreed let's not do that as we are at risk of creating an infinite loop in the useEffect.

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

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

TrySound · 2023-08-03T17:10:17Z

@cdedreuille @JReinhold icons packages depends on very old svgr and svgo versions with many vulnerable or deprecated packages. And why it even depends on svgr? Isn't it a collection of prebuilt icons?

TrySound · 2023-08-03T17:12:06Z

https://packagephobia.com/result?p=%40storybook%2Ficons

cdedreuille · 2023-08-03T17:12:54Z

@TrySound it should not depend on svgr. We are only using it to generate icons from Figma.

TrySound · 2023-08-03T17:15:02Z

So none of these dependencies are really needed?
https://github.com/storybookjs/icons/blob/main/package.json#L99-L106

cdedreuille · 2023-08-03T17:41:42Z

You’re right. We need to move them into devDependencies.

Ref storybookjs/storybook#23635 (comment) Looks like all dependencies are build only and should not bloat user installation with deprecated and vulnerable packages.

TrySound · 2023-08-03T21:13:36Z

storybookjs/icons#14

…trols-addon" This reverts commit 0005430, reversing changes made to a70dcab.

MWhite-22 · 2023-09-19T15:32:26Z

code/ui/blocks/src/components/ArgsTable/ArgControl.tsx

+  if (!control || control.disable)
+    return isHovered ? (
+      <Link
+        href="https://storybook.js.org/docs/react/essentials/controls"
+        target="_blank"
+        withArrow
+      >
+        Setup controls
+      </Link>
+    ) : (


@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.

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.

@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?

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.

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.

Improve loading for controls

ea1b224

cdedreuille added maintenance User-facing maintenance tasks ci:normal patch:no labels Jul 27, 2023

cdedreuille added 8 commits July 28, 2023 09:21

Merge branch 'next' into charles-update-controls-addon

cc19cff

Made loading works

fd67d96

Fix types error

b6cdb0c

Update ControlsPanel.tsx

997abea

Add Link component

2f9ce19

Update

81d6bd9

Fix links

4a717d9

Improve Link + Icons

3ed5f7a

cdedreuille added 10 commits July 31, 2023 17:49

Icons are now working :)

89ecd7e

Improve Link component

75935b9

Improve Link

d156fb9

Merge branch 'next' into charles-update-controls-addon

dbd77c8

Improve link

a3a6327

Update Empty.tsx

22cfba6

Remove FakeIcon

716064c

Add hover link when not set up

f524837

Update Link.tsx

e5734eb

Fix some icons names

90eb841

cdedreuille marked this pull request as ready for review August 1, 2023 07:36

cdedreuille requested review from ndelangen, JReinhold, tmeasday and kasperpeulen as code owners August 1, 2023 07:36

cdedreuille self-assigned this Aug 1, 2023

yannbf approved these changes Aug 1, 2023

View reviewed changes

cdedreuille added 6 commits August 1, 2023 17:47

Update ControlsPanel.tsx

e206d21

Merge branch 'next' into charles-update-controls-addon

fa6d316

Merge branch 'next' into charles-update-controls-addon

df56d8b

Merge branch 'next' into charles-update-controls-addon

f3d2828

Merge branch 'next' into charles-update-controls-addon

0ed119c

Fix the Link style

8a5ce35

JReinhold requested changes Aug 2, 2023

View reviewed changes

Add delay to empty state to prevent flickering

b22fd14

cdedreuille requested a review from JReinhold August 2, 2023 08:31

JReinhold approved these changes Aug 2, 2023

View reviewed changes

JReinhold merged commit 0005430 into next Aug 2, 2023
59 checks passed

JReinhold deleted the charles-update-controls-addon branch August 2, 2023 09:02

github-actions bot mentioned this pull request Aug 2, 2023

Release: Patch 7.2.1 #23674

Merged

14 tasks

TrySound added a commit to TrySound/icons that referenced this pull request Aug 3, 2023

Move all deps to dev deps

d2f8113

Ref storybookjs/storybook#23635 (comment) Looks like all dependencies are build only and should not bloat user installation with deprecated and vulnerable packages.

TrySound mentioned this pull request Aug 3, 2023

Move all deps to dev deps storybookjs/icons#14

Merged

ndelangen added a commit that referenced this pull request Aug 21, 2023

Revert "Merge pull request #23635 from storybookjs/charles-update-con…

df5b104

…trols-addon" This reverts commit 0005430, reversing changes made to a70dcab.

MWhite-22 reviewed Sep 19, 2023

View reviewed changes

vanessayuenn mentioned this pull request Dec 28, 2023

[Bug]: Cannot disable "Args table with interactive controls couldn't be auto-generated" panel #24422

Closed

yannbf mentioned this pull request Jan 2, 2024

Addon Controls: Remove unused hideNoControlsWarning type #25417

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Improve controls addon #23635

UI: Improve controls addon #23635

cdedreuille commented Jul 27, 2023 •

edited

socket-security bot commented Jul 31, 2023 •

edited

socket-security bot commented Jul 31, 2023 •

edited

JReinhold left a comment

cdedreuille commented Aug 2, 2023

JReinhold commented Aug 2, 2023

cdedreuille commented Aug 2, 2023

cdedreuille commented Aug 2, 2023 •

edited

JReinhold left a comment

JReinhold Aug 2, 2023

cdedreuille Aug 2, 2023

JReinhold Aug 2, 2023

cdedreuille Aug 2, 2023

yannbf Aug 2, 2023

cdedreuille Aug 2, 2023

TrySound commented Aug 3, 2023

TrySound commented Aug 3, 2023

cdedreuille commented Aug 3, 2023

TrySound commented Aug 3, 2023 •

edited

cdedreuille commented Aug 3, 2023

TrySound commented Aug 3, 2023

MWhite-22 Sep 19, 2023 •

edited

MWhite-22 Sep 19, 2023 •

edited

cdedreuille Sep 20, 2023

MWhite-22 Sep 20, 2023

cdedreuille Sep 22, 2023

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

UI: Improve controls addon #23635

UI: Improve controls addon #23635

Conversation

cdedreuille commented Jul 27, 2023 • edited

What I did

How to test

Checklist

Maintainers

🦋 Canary release

socket-security bot commented Jul 31, 2023 • edited

socket-security bot commented Jul 31, 2023 • edited

JReinhold left a comment

Choose a reason for hiding this comment

cdedreuille commented Aug 2, 2023

JReinhold commented Aug 2, 2023

cdedreuille commented Aug 2, 2023

cdedreuille commented Aug 2, 2023 • edited

JReinhold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TrySound commented Aug 3, 2023

TrySound commented Aug 3, 2023

cdedreuille commented Aug 3, 2023

TrySound commented Aug 3, 2023 • edited

cdedreuille commented Aug 3, 2023

TrySound commented Aug 3, 2023

MWhite-22 Sep 19, 2023 • edited

Choose a reason for hiding this comment

MWhite-22 Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdedreuille commented Jul 27, 2023 •

edited

socket-security bot commented Jul 31, 2023 •

edited

socket-security bot commented Jul 31, 2023 •

edited

cdedreuille commented Aug 2, 2023 •

edited

TrySound commented Aug 3, 2023 •

edited

MWhite-22 Sep 19, 2023 •

edited

MWhite-22 Sep 19, 2023 •

edited