-
Notifications
You must be signed in to change notification settings - Fork 116
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
feat(icons): make decorative required + icon docs #165
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/twilio-dsys/paste/b0irz0j3b |
This looks good to me. Are we planning on building a page on the docs site showing all of the icons available? I know we do in Storybook, but that might be useful here as well. I didn't see a ticket in our backlog for that, but maybe I missed it. |
Good point! For now, I'll just link to Storybook from the docs page. I'm not sure if it's valuable to list out all the components in both places just yet. I'd prefer to do that duplication work when the system matures a little more. |
So I think this page should live under components as the icon component page, and this page in the root of the website IA should become the overall list of icons |
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.
LGTM other than these changes
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.
lgtm! The changes I care most about are:
- Removing
size="icon"
from that one live component example - Adding the word "Avoid" to the body of the don'ts
Everything else is minor.
</SiteNavItem> | ||
<SiteNavItem> | ||
<SiteNavAnchor to={`${SidebarCategoryRoutes.ICON_SYSTEM}/how-to-add-an-icon`}> | ||
How to add an icon |
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.
Seems like there's some automatic title casing going on in the sidebar, which makes it a little weird that the H1 is sentence case, and the sidebar item is title case.
Not important for this PR but noting it for the future.
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.
Should I leave it, remove title casing on the sidebars, add it to h1s, or do nothing? None of these options is too huge a change code-wise, but it may have an outsized impact design-wise.
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.
Do nothing for now. It'd be ideal to remove the title casing from the sidebars, but I'm worried we'll end up with some unintentional casing there.
packages/paste-website/src/pages/icon-system/how-to-add-an-icon.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/icon-system/how-to-add-an-icon.mdx
Outdated
Show resolved
Hide resolved
A title is required so assistive technology can infer equal meaning as a sighted user would. | ||
|
||
<DoDont> | ||
<Do title="Do" body="Use 'title' text that gives context and meaning to the icon." center> |
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.
"title" in these do/don'ts should use the code text style
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.
Our current implementation of DoDont makes that impossible. Going to leave as is for now.
packages/paste-website/src/pages/icon-system/how-to-add-an-icon.mdx
Outdated
Show resolved
Hide resolved
packages/paste-website/src/pages/icon-system/how-to-add-an-icon.mdx
Outdated
Show resolved
Hide resolved
Just a few comments:
|
@serifluous Ready for re-review! |
of a page. The information provided by the image might already be given using adjacent text, or the image | ||
might be included to make the website more visually attractive."* (<Anchor href="https://www.w3.org/WAI/tutorials/images/decorative/">w3.org</Anchor>) | ||
|
||
Our icons require the developer define whether an icon is decorative by [providing the required property](/icon-system#making-an-icon-decorative-or-informative). |
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.
Our icons require the developer define whether an icon is decorative by [providing the required property](/icon-system#making-an-icon-decorative-or-informative). | |
Our icons require the developer to define whether an icon is decorative by [providing the required property](/icon-system#making-an-icon-decorative-or-informative). |
|
||
#### Adapting the icon's color | ||
|
||
We can change the icon color directly using [text color tokens](/tokens/#text-colors). |
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.
Should we phrase this like the Resizing icons text below, i.e.:
We can change the icon color directly using our textColor
tokens.
decorative
prop required on all icons.