UI: Fix RefIndicator to use CheckIcon instead of string #29209

JSMike · 2024-09-25T14:36:57Z

Currently there's a bug in the composed storybook version picker where the currently selected version has the word 'check' in front of it instead of a check icon.

https://github.com/storybookjs/storybook/blob/v8.3.3/code/core/src/manager/components/sidebar/RefIndicator.tsx#L219
https://github.com/storybookjs/storybook/blob/v8.3.3/code/core/src/components/components/tooltip/TooltipLinkList.tsx#L54
https://github.com/storybookjs/storybook/blob/v8.3.3/code/core/src/components/components/tooltip/TooltipLinkList.tsx#L41
https://github.com/storybookjs/storybook/blob/v8.3.3/code/core/src/components/components/tooltip/ListItem.tsx#L177
https://github.com/storybookjs/storybook/blob/v8.3.3/code/core/src/components/components/tooltip/ListItem.tsx#L206

What I did

Fix bug in refIndicator where reactNode is expected but string is passed.
Import CheckIcon from @storybook/icons and replace string 'check' with CheckIcon.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Create a sandbox: yarn task --task sandbox --template react-vite/default-ts
Modify ./sandbox/react-vite-default-ts/.storybook/main.ts:

Add key to exports:

refs: {  test: { title: 'test', url: 'http://localhost:8080'  } },

Build sandbox:

cd sandbox/react-vite-default-ts
yarn build-storybook

Create metadata.json:

in code editor go to ./sandbox/react-vite-default-ts/storybook-static
create file 'metadata.json' with contents:

{
  "versions": {
    "v0": "http://localhost:8080"
  }
}

Serve storybook-static: (this must be running before starting the other storybook)

cd sandbox/react-vite-default-ts/storybook-static
npx http-server . --cors

Run sandbox:

cd sandbox/react-vite-default-ts
yarn storybook

Open Storybook in your browser
Scroll to bottom of sidebar to see composed storybook
Select version picker dropdown for composed storybook. Expect to see checkmark instead of word 'check'.

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for 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:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 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>

name	before	after	diff	z	%
createSize	0 B	0 B	0 B	-	-
generateSize	78.5 MB	78.5 MB	0 B	2.92	0%
initSize	151 MB	151 MB	3.4 kB	-0.55	0%
diffSize	72.6 MB	72.6 MB	3.4 kB	-0.71	0%
buildSize	6.77 MB	6.77 MB	34 B	-1.09	0%
buildSbAddonsSize	1.5 MB	1.5 MB	0 B	-1.11	0%
buildSbCommonSize	195 kB	195 kB	0 B	-	0%
buildSbManagerSize	1.83 MB	1.83 MB	34 B	-1.07	0%
buildSbPreviewSize	270 kB	270 kB	0 B	-1.11	0%
buildStaticSize	0 B	0 B	0 B	-	-
buildPrebuildSize	3.8 MB	3.8 MB	34 B	-1.09	0%
buildPreviewSize	2.97 MB	2.97 MB	0 B	-1.45	0%
testBuildSize	0 B	0 B	0 B	-	-
testBuildSbAddonsSize	0 B	0 B	0 B	-	-
testBuildSbCommonSize	0 B	0 B	0 B	-	-
testBuildSbManagerSize	0 B	0 B	0 B	-	-
testBuildSbPreviewSize	0 B	0 B	0 B	-	-
testBuildStaticSize	0 B	0 B	0 B	-	-
testBuildPrebuildSize	0 B	0 B	0 B	-	-
testBuildPreviewSize	0 B	0 B	0 B	-	-

name	before	after	diff	z	%
createTime	16.7s	21.2s	4.4s	1.12	20.8%
generateTime	18.7s	20s	1.2s	-0.31	6.4%
initTime	13.2s	13.7s	528ms	-0.65	3.8%
buildTime	8.8s	9s	140ms	-0.75	1.6%
testBuildTime	0ms	0ms	0ms	-	-
devPreviewResponsive	8.3s	7.2s	-1s -19ms	0.05	-14%
devManagerResponsive	5.7s	4.4s	-1s -305ms	-0.3	-29.5%
devManagerHeaderVisible	545ms	640ms	95ms	0.11	14.8%
devManagerIndexVisible	574ms	670ms	96ms	0.07	14.3%
devStoryVisibleUncached	1.2s	1.3s	146ms	0.46	10.7%
devStoryVisible	573ms	683ms	110ms	0.16	16.1%
devAutodocsVisible	459ms	619ms	160ms	0.88	25.8%
devMDXVisible	455ms	581ms	126ms	0.34	21.7%
buildManagerHeaderVisible	532ms	572ms	40ms	0.02	7%
buildManagerIndexVisible	560ms	612ms	52ms	0.12	8.5%
buildStoryVisible	562ms	611ms	49ms	-0.05	8%
buildAutodocsVisible	490ms	574ms	84ms	0.14	14.6%
buildMDXVisible	538ms	540ms	2ms	0.09	0.4%

greptile-apps

LGTM

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

nx-cloud · 2024-09-25T14:40:57Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1c22d65. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

JSMike · 2024-09-25T14:43:05Z

Note: I have identified the root cause of the issue that I outlined. I'm not 100% that this is the correct solution, I've had issues setting up a versioned composed storybook in a sandbox. If anyone has steps to create a sandbox with a versioned composed storybook that would be appreciated. I'll update the manual steps in the PR once I have confirmed.

I've now confirmed the fix.

JSMike · 2024-09-30T16:14:17Z

FYI, pushed another commit, this is now 'working as intended' but you may prefer a different implementation. Let me know if you'd like any changes:

Before:

After:

JSMike · 2024-09-30T16:22:32Z

Updated manual test steps

JSMike · 2024-09-30T16:26:43Z

code/core/src/components/components/tooltip/ListItem.tsx

@@ -199,11 +199,17 @@ const ListItem = ({
 }: ListItemProps) => {
  const commonProps = { active, disabled };
  const itemProps = getItemProps(onClick, href, LinkWrapper);
+  const Icon: React.JSXElementConstructor<any> =


I updated the logic to render any icon that's passed forward. This was the easiest way to type a generic component, let me know if you'd like a different implementation.

Looks like this change broke some things, working on resolving.

JSMike · 2024-10-03T12:56:51Z

Confirmed simpler fix based on https://github.com/JSMike/storybook/blob/next/code/core/src/components/components/tooltip/TooltipLinkList.stories.tsx#L62

same manual steps to reproduce:

greptile-apps bot reviewed Sep 25, 2024

View reviewed changes

JSMike changed the title ~~UI: Fix refIndicator to use ReactNode for check icon~~ UI: Fix refIndicator to use CheckIcon instead of string Sep 26, 2024

JSMike changed the title ~~UI: Fix refIndicator to use CheckIcon instead of string~~ UI: Fix RefIndicator to use CheckIcon instead of string Sep 26, 2024

kasperpeulen assigned kasperpeulen and shilman and unassigned kasperpeulen Sep 30, 2024

kasperpeulen added ci:normal bug ui composition labels Sep 30, 2024

JSMike commented Sep 30, 2024

View reviewed changes

UI: Fix refIndicator to use ReactNode for check icon

1c22d65

JSMike force-pushed the fix-refIndicator-icon branch from 48868ba to 1c22d65 Compare October 3, 2024 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Fix RefIndicator to use CheckIcon instead of string #29209

UI: Fix RefIndicator to use CheckIcon instead of string #29209

JSMike commented Sep 25, 2024 •

edited

Loading

greptile-apps bot left a comment

nx-cloud bot commented Sep 25, 2024 •

edited

Loading

JSMike commented Sep 25, 2024 •

edited

Loading

JSMike commented Sep 30, 2024 •

edited

Loading

JSMike commented Sep 30, 2024

JSMike Sep 30, 2024 •

edited

Loading

JSMike Oct 3, 2024

JSMike commented Oct 3, 2024 •

edited

Loading

UI: Fix RefIndicator to use CheckIcon instead of string #29209

Are you sure you want to change the base?

UI: Fix RefIndicator to use CheckIcon instead of string #29209

Conversation

JSMike commented Sep 25, 2024 • edited Loading

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

greptile-apps bot left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Sep 25, 2024 • edited Loading

☁️ Nx Cloud Report

JSMike commented Sep 25, 2024 • edited Loading

JSMike commented Sep 30, 2024 • edited Loading

JSMike commented Sep 30, 2024

JSMike Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

JSMike Oct 3, 2024

Choose a reason for hiding this comment

JSMike commented Oct 3, 2024 • edited Loading

JSMike commented Sep 25, 2024 •

edited

Loading

nx-cloud bot commented Sep 25, 2024 •

edited

Loading

JSMike commented Sep 25, 2024 •

edited

Loading

JSMike commented Sep 30, 2024 •

edited

Loading

JSMike Sep 30, 2024 •

edited

Loading

JSMike commented Oct 3, 2024 •

edited

Loading