UI: Upgrade Icon component #23680
UI: Upgrade Icon component #23680
Conversation
cdedreuille
changed the title
cdedreuille
requested review from
ndelangen,
JReinhold,
tmeasday,
yannbf,
valentinpalkovic,
kylegach and
jonniebigodes
as code owners
There was a problem hiding this comment.
@cdedreuille thanks for taking on this task and updating the snippets we're using. Appreciate it 🙏 ! The documentation changes look good to me, but I'll defer to the rest of the team to see what they say.
So that everyone is aware, I've already followed up with Charles to not only add some changes to the documentation but also a reminder for the addon kit to be updated to factor in these ones as well to prevent the code & documentation fall out of sync.
| @@ -109,7 +109,8 @@ Going through the code blocks in sequence: | |||
| ```ts | |||
| import { useGlobals, useStorybookApi } from '@storybook/manager-api'; | |||
|
|
|||
| import { Icons, IconButton } from '@storybook/components'; | |||
| import { IconButton } from '@storybook/components'; | |||
| import { Icon } from '@storybook/components/experimental'; | |||
There was a problem hiding this comment.
do we want addon maintainers to do this, given that it's experimental?
There was a problem hiding this comment.
Ideally yes. It is experimental mostly because we want to move this at some point to a core package. @ndelangen can talk more about that. But the components inside this package are the real one and should be used.
| @@ -144,7 +145,7 @@ export const VisionSimulator = () => { | |||
| onDoubleClick={() => setFilter(null)} | |||
| > | |||
| <IconButton key="filter" active={!!filter} title="Vision simulator"> | |||
| <Icons icon="accessibility" /> | |||
| <Icon.Accessibility /> | |||
There was a problem hiding this comment.
According to the docs you wrote:
The meaning can be obvious visually, but it must have a proper text alternative via
aria-labelfor screen readers
Should you make sure every icon in this PR has the proper aria-label applied to it?
There was a problem hiding this comment.
@yannbf That would be great but that would require quite some time to test everything. I would put that effort in a separate PR though with other efforts to improve accessibility.
There was a problem hiding this comment.
I found a new way to help accessibility in a semi automated way @yannbf. I'll explore this more in a separate PR.
yannbf
changed the title
shilman
changed the title
What I did
We introduced a new library for icons. In this PR I'm replacing all icons by the new library. This PR doesn't include the following:
This PR also fixes:
How to test
yarn task --task sandbox --start-from auto --template react-vite/default-tsChecklist
MIGRATION.MD
Maintainers
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts["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/coreteam 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>