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

Update icon styling #5432

Open
elzannewentzel opened this issue Jun 6, 2022 · 9 comments
Open

Update icon styling #5432

elzannewentzel opened this issue Jun 6, 2022 · 9 comments
Assignees
Labels
design refresh Issues connected to the Design Refresh of 2021 enhancement sprint-rollover

Comments

@elzannewentzel
Copy link
Contributor

elzannewentzel commented Jun 6, 2022

Update the icon library we use on the platform (maybe)
Update the styling of all icons on the platform:

@elzannewentzel elzannewentzel added this to the A - Sprint 6 milestone Jun 6, 2022
@bretthayes bretthayes self-assigned this Jun 6, 2022
@zlonko zlonko assigned zlonko and unassigned bretthayes Jun 9, 2022
@zlonko
Copy link
Contributor

zlonko commented Jun 13, 2022

In alignment with the Figmas, we can use this ticket to implement a consolidated icon library. I am gathering Tracey's feedback here, otherwise, this is nearly ready for review.

@zlonko
Copy link
Contributor

zlonko commented Jun 13, 2022

Documenting a discussion with @traceyljohnson:

Initially, we proposed react-icon library for consolidation and consistency. However, brand design would like to align About with the product by focusing on Sharp-style Material icons. Some icons used in new Figma prototypes are not present in react-icon.

Because we are focusing on Material icons in the future, it seems we can opt for the MUI library (search, npm) instead of a larger library.

  • This ensures we use icons of the preferred style
  • This reduces our flexibility and increases our dependency on the big G
  • Reminder that Material includes non-illustrative icons, such as the logos for GitHub and Twitter that we use on About

@bretthayes @st0nebraker: What are your thoughts? I recognize that this choice has tradeoffs, so I am open to alternatives.

@zlonko zlonko mentioned this issue Jun 13, 2022
@bretthayes
Copy link
Contributor

I think that's a great idea to align brand between our CPT web properties and the product. Having brand consistency throughout all of our codebases would be a fantastic alignment! @traceyljohnson

@zlonko Would you mind looking into the bundle size that MUI brings and if there's any dependencies other than the @material-ui/icons scoped package? I'm curious what this will add to our bundle size.

Something to think about is standardizing our common Icon component with convenient props as well. I was thinking something along the lines of this:

import { Icon } from '@components'

<Icon name="CheckCircle" size={48} />

What do you think?

@zlonko
Copy link
Contributor

zlonko commented Jun 14, 2022

Sure thing, @bretthayes, I will look into the bundle size and possible dependencies. I agree that we should have an Icon component to centralize the styling for these icons and reduce existing redundancy. Here is a link to the WIP.

@zlonko
Copy link
Contributor

zlonko commented Jun 14, 2022

@bretthayes I dug in a little more and it looks like @material-ui/icons does not include all of the icons we need.

A little confusing: the search console actually links to @mui/icons-material and not @material-ui/icons. This package includes all of the icons available to Tracey, so I went with this one. It lists @babel/runtime as its only dependency.

According to next/bundle-analyzer, it is also humongous compared to mdi-react. This definitely will decrease performance so I am open to what you suggest!

mui/icons-material: parsed size is 4.08MB

Screen Shot 2022-06-14 at 3 05 22 PM

mdi-react: parsed size is 17.7KB

Screen Shot 2022-06-14 at 3 07 15 PM

@bretthayes
Copy link
Contributor

Oh wow! That's quite a big difference! We should investigate how to refine that and also test to make sure tree shaking is working for our production builds. Afaik, Next.js and MUI both support tree shaking so only the imported icons should be added to the bundle size. Is this a dev build or a prod build? Something seems off here, so there may be some config involved.

A little confusing: the search console actually links to @mui/icons-material and not @material-ui/icons.

It looks like the @material-ui/icons is from the v4 docs and the @mui/icons-material is from the v5 docs so you have the updated one. 👌🏻

Screen Shot 2022-06-15 at 10 58 07 AM

@bretthayes bretthayes modified the milestones: A - Sprint 6, A - Sprint 7 Jun 20, 2022
@bretthayes bretthayes self-assigned this Jul 5, 2022
@st0nebreaker st0nebreaker added the design refresh Issues connected to the Design Refresh of 2021 label Jul 7, 2022
@bretthayes
Copy link
Contributor

I think we should re-consider not using MUI and look for a lighter UI library to use because it's causing rendering problems and bloat in our repo with the extra dependencies. The whole site is loading much slower now and I don't want this to reduce the performance of the site after all the hard work we've done to increase performance.

@traceyljohnson can we look into alternative UI libraries together?

cc: @elzannewentzel @fabicastp

@traceyljohnson
Copy link
Contributor

@bretthayes Yeah, that's no good. Let me know what I can do to help!

@bretthayes
Copy link
Contributor

Just following up on this with some context. We have the react-icons package available to us (see comment) in the repo which allows us to reference many different icon libraries. However, in order to align with Brand, we are continuing to mainly use the material UI icon library with the added benefit of being able to reference other libraries on a need-be basis.

In order to complete this ticket and instead of the previous approach of importing all icons, we should instead:

  • create an Icon component that allows us to pass the name of a valid icon from the material UI react-icons library. This will scope the component with the available icons we should use to meet our brand-aligned library of choice, and;
  • add styling and props as defined in our DLS. This will give us the consistent styling we're trying to achieve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design refresh Issues connected to the Design Refresh of 2021 enhancement sprint-rollover
Projects
Status: Blocked/Paused
Development

Successfully merging a pull request may close this issue.

6 participants