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

🧹 [UI House Keeping] Icon #16186

Closed
16 of 19 tasks
georgewrmarshall opened this issue Oct 13, 2022 · 2 comments · Fixed by #16621
Closed
16 of 19 tasks

🧹 [UI House Keeping] Icon #16186

georgewrmarshall opened this issue Oct 13, 2022 · 2 comments · Fixed by #16621
Assignees
Labels
area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension

Comments

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Oct 13, 2022

Description

Ensure that Icon adheres to all of the following conventions and standards

  • Ensure that the url for icon is being rendered correctly in the html
  • Has a className prop and the PropType descriptions are all the same
  • Prop table in MDX docs have the "Accepts all Box component props" description and link
  • We are consistent when using the same prop names like size and are suggesting the use of the generalized design-system.js constants e.g. SIZES as the primary option but noting the component consts in the documentation and using them for propType validation and storybook controls only
  • Standardize all similar prop names for images imgSrc, imgAlt(html element + attribute) (needs audit)
  • We have a story for each component prop and we use the prop name verbatim e.g. size prop would be export const Size = (args) => (
  • We have the accompanying documentation for each component prop and we use the prop name verbatim e.g. size prop would be ### Size
  • Are multiple props stories allowed? e.g. Color, Background Color And Border Color story in base-avatar - [ ] yes when it makes sense to
  • All Base components follow the suffix convention e.g. ButtonBase
  • All Base component MDX documentation have the base component notification at the top
  • Add mm- prefix to all classNames
  • className is kebab case version of the component name
  • Spread base components props and reduce duplication of props when props aren't being changed and remain the same for both variant and base components
  • Add component to root index.js file in component-library
  • Add locals for any default text I18nContext as default context
  • Add any "to dos" with a // TODO: comment so we can search for them at a later date e.g. blocking components etc
  • Add snapshot testing
  • Add pixel values to propType descriptions if we use abstracted prop types that relate to pixel values e.g. SIZE.MD (32px)
  • Each prop section in the MDX docs should have: a heading, a description, a story and an example code snipped
@georgewrmarshall georgewrmarshall added area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension IA/NAV labels Oct 13, 2022
@georgewrmarshall georgewrmarshall self-assigned this Oct 13, 2022
@garrettbear
Copy link
Contributor

When doing your house keeping, let's also fix the closing tags of the Icon component.

https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/component-library/icon/icon.js#L31

maskImage: `url('./images/icons/icon-${name}.svg')`,
WebkitMaskImage: `url('./images/icons/icon-${name}.svg')`,

FYI: This will break some snapshots.

@georgewrmarshall
Copy link
Contributor Author

@garrettbear do you mean add the semi colon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants