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] Synchronize components with Google #19485

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 30, 2020

Related to #19483

I couldn't spot any issue.

@oliviertassinari oliviertassinari added the package: icons Specific to @mui/icons label Jan 30, 2020
@mui-pr-bot
Copy link

No bundle size changes comparing bdebdfd...35d42b1

Generated by 🚫 dangerJS against 35d42b1

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 30, 2020

@mbrookes To follow-up on your question regarding two-tones, it seems we can leverage the opacity to identify the secondary layer:

Capture d’écran 2020-01-30 à 22 53 37
Capture d’écran 2020-01-30 à 22 53 29

Then looking at https://fontawesome.com/how-to-use/on-the-web/styling/duotone-icons, we would need to apply a class name on the secondary layer, so people can style it differently. Then, we could either use the fill vs color trick or CSS variable or leave to userland to apply the right color (maybe better).

@oliviertassinari
Copy link
Member Author

@timmydoza Does it look good to you?

@timmydoza
Copy link
Contributor

@timmydoza Does it look good to you?

Looks good!! But I still see some noise in there. If you search for opacity=".87" you'll find a number of things. For instance:

<g fill="none"><path d="M0 0h24v24H0V0z" /><path d="M0 0h24v24H0V0z" opacity=".87" /></g>

^^ That's one of a few variations that I found.

To look for more, I generated the docs from your branch. I went to the icon page and added this (hacky) css in devtools:

* {
  stroke: red;
}

It helps to make 'noise' visible so you can see which icons are affected:

image

The icons with the boxes around them probably have some noise in there.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 30, 2020

Smart! I haven't thought of using this CSS to find all the "noisy" icons. I haven't tried to fix the stroke issues, I leave it to you if you shall accept it :).

My only concern was to sync with the latest version of the icons. You can preview it too at https://deploy-preview-19485--material-ui.netlify.com/components/material-icons/#material-icons.

@timmydoza
Copy link
Contributor

Smart! I haven't thought of using this CSS to find all the "noisy" icons. I haven't tried to fix the stroke issues, I leave it to you if you shall accept it :).

I'll consider it! Right now there are just a few icons that I need to be 'noiseless'.

My only concern was to sync with the latest version of the icons. You can preview it too at https://deploy-preview-19485--material-ui.netlify.com/components/material-icons/#material-icons.

I don't see any obvious issues as far as the sync goes. However, it's too many icons for me to manually verify everything 😄 .

@oliviertassinari
Copy link
Member Author

I'll consider it! Right now there are just a few icons that I need to be 'noiseless'.

Fair enough, at least, we are happy to see the number goes down!

I don't see any obvious issues as far as the sync goes. However, it's too many icons for me to manually verify everything 😄 .

Same concern on my end. I have tried running a visual diff with no luck as we have a couple of new icons that shifts everything. But scanning all the icons, it looked OK.

Capture d’écran 2020-01-31 à 00 24 56

@timmydoza
Copy link
Contributor

Fair enough, at least, we are happy to see the number goes down!

Once this gets merged I'll add more noise snippets to the build script in my branch. It shouldn't take much to get rid of most of the noise. 👍

@oliviertassinari oliviertassinari merged commit 5f990d4 into mui:master Jan 31, 2020
@oliviertassinari oliviertassinari deleted the update-icons branch January 31, 2020 19:05
@oliviertassinari
Copy link
Member Author

You are good for a rebase :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: icons Specific to @mui/icons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants