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

Icons test #1466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Icons test #1466

wants to merge 1 commit into from

Conversation

renrizzolo
Copy link
Contributor

@renrizzolo renrizzolo commented Aug 10, 2022

Description

This PR is for testing only, to make cleaning up existing icon svgs easier. Icons won't necessarily be in this repo.

  • add icons list to docs for testing purposes
  • add icon generation pipeline
    • svg files in icons/src/svg are converted to react components
    • runs through svgo with our existing config
    • replaces fill with currentColor (currently as color prop that would be overridable; tbd)
    • adds a size prop that maps to svg width and height
    • adds svg-icon default className and className prop

These could be used directly:

import { Adunit } from 'icons'

<Adunit/>

or

import * as Icons from 'icons'

<Icons.Adunit/>
  • add generic Icon component
    • includes typing of name prop for all available icons (could map this back to kebab case to keep existing API)
      These would be used like this:
import Icon from 'icon'

<Icon name="Adunit"/>

not sure of the best API here, but a few things to consider/investigate

  • tree shakeability
  • importing/intellisense and usage ergonomics
  • ease of migration for existing component

@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #1466 (a6876e0) into master (8078b67) will not change coverage.
The diff coverage is n/a.

❗ Current head a6876e0 differs from pull request most recent head 789aad4. Consider uploading reports for the commit 789aad4 to get more accurate results

@@            Coverage Diff            @@
##            master     #1466   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           96        97    +1     
  Lines         1703      1604   -99     
  Branches       477       452   -25     
=========================================
- Hits          1703      1604   -99     

see 14 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 637 Code Smells

No Coverage information No Coverage information
1.8% 1.8% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jan 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 300 Code Smells

No Coverage information No Coverage information
1.8% 1.8% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Mar 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 300 Code Smells

No Coverage information No Coverage information
1.7% 1.7% Duplication

Comment on lines +38 to +39
"generate-icon-names": "node icons/src/generateIconNames.js",
"convert-icons": "npm run clean-icons && npx @svgr/cli --out-dir icons/src/react --ext=jsx --template=icons/template.js --expand-props=none --svg-props width={size} --svg-props height={size} --replace-attr-values=#5a5a5a={color},#333={color},currentColor={color} -- icons/src/svg && npm run generate-icon-names",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main transformation script.
We don't need the generate-icon-names script if we choose to not use the <Icon name="..."/> component.

The main thing to note here is
--replace-attr-values=#5a5a5a={color}
if all the source svgs have the same fill colour (e.g #5a5a5a), we can use this to replace the fill with currentColor string or a {color} prop.

the template.js file has the markup for the props etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant