Skip to content
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

Merged
merged 25 commits into from
Nov 20, 2019
Merged

Conversation

TheSisb
Copy link
Contributor

@TheSisb TheSisb commented Nov 7, 2019

  • Added documentation for Paste Icons.
  • Made the decorative prop required on all icons.

@vercel
Copy link

vercel bot commented Nov 7, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/twilio-dsys/paste/b0irz0j3b
🌍 Preview: https://paste-git-docs-icons.twilio-dsys.now.sh

@TheSisb TheSisb self-assigned this Nov 7, 2019
@TheSisb TheSisb requested review from SiTaggart, richbachman, aayushpi and a team and removed request for aayushpi November 7, 2019 17:11
@TheSisb TheSisb added Area: Components Related to the component library (core) of this system Area: Doc Site Related to the documentation website Status: Pls CR This PR is ready for Code Reviews Type: Documentation Improvements or additions to documentation labels Nov 7, 2019
@TheSisb TheSisb changed the title feat(website): icon docs feat(icons): make decorative required + icon docs Nov 7, 2019
@TheSisb TheSisb marked this pull request as ready for review November 7, 2019 17:57
@richbachman
Copy link
Contributor

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.

@TheSisb
Copy link
Contributor Author

TheSisb commented Nov 8, 2019

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.

@SiTaggart
Copy link
Contributor

Are we planning on building a page on the docs site showing all of the icons available?

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

packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icons/index.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@aayushpi aayushpi left a 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

packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
Copy link
Member

@serifluous serifluous left a 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.

packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
</SiteNavItem>
<SiteNavItem>
<SiteNavAnchor to={`${SidebarCategoryRoutes.ICON_SYSTEM}/how-to-add-an-icon`}>
How to add an icon
Copy link
Member

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.

Copy link
Contributor Author

@TheSisb TheSisb Nov 19, 2019

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.

Copy link
Member

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/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.mdx Outdated Show resolved Hide resolved
packages/paste-website/src/pages/icon-system/index.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>
Copy link
Member

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

Copy link
Contributor Author

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.

@laijinfong
Copy link

laijinfong commented Nov 19, 2019

Just a few comments:

  • Feels like the padding between the icon and copy on a button is very tight
  • In the section where you say hover the icon, i'd divorce that away from the icon you want to show the tooltip and put it in the bottom left/bottom right as it looks like it's part of the design/example
  • I also suggest changing the text to say 'Hover over the icon for a tool tip example' or something along those lines and being more descriptive
  • The padding is really tight between the Now Loading example, padding is also really tight from the icon and the edge of the container
  • In the resizing icons section, the icons are also really tightly placed between one another, we need some padding between the icons
  • Might be a button component issue rather than the icon issue but I think it's related, but I find the padding between the end of the string very tight, and also the padding between the icon and string really tight. We might want to consider adding more space between them; looks fine on sketch because the icon had padding around it but if we are going to automate it on a systems level, the icon needs to be in a container. otherwise, the user will need to recreate a new button each time if they want a new button.

@TheSisb
Copy link
Contributor Author

TheSisb commented Nov 19, 2019

@serifluous Ready for re-review!
@laijinfong Still working through your notes, thanks!

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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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).
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Components Related to the component library (core) of this system Area: Doc Site Related to the documentation website Status: Pls CR This PR is ready for Code Reviews Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants