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: changed docs to use new icons and removed old icons #2006

Merged
merged 5 commits into from
Mar 20, 2023

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Mar 10, 2023

Fixes #1999

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

This PR deals with removing all old icons and using the new icons in our components and code.
There are several points of interest in this PR
note Since this is such a large PR and touches many components, I want people to take a look at this, and make sure this approach is correct, as well as if there is something that I msised. I looked through it pretty thoroughly, but as always, there could be gaps.
After looking at it and it seems okay, then I will run a percy build to verify the changes are correct. I don't want to run it too often, since it will need to run a full build.

Notes

  • All aliases have been removed. We are using the base icons for those. However, several aliases have been kept around (and we can discuss about if we should remove them or such)
    Icons kept around
eek-range
eek-arrow
icon-stepper-{insert state here}
star-rating (both dynamic and static)
radio (all states)
checkbox (all states)

Stepper: I kept around only for consistency. There are some stepper icons present in the base icons, but there are some which are not (namely the complete and in progress step). I'm fine updating it to use a mix.
EEK: They don't have sizes associated with thm
checkbox/radio: I could set a size for them, but they would mismatch with the base icons. Also they're not present in design system icons.

  • All icons for RTL are rotated by their specific components and no longer at a global level.
  • Since they have two overflow icons now, one ios and android, I used those in the correct locations and no longer rotate the normal overflow icon. I personally dislike the name (we should probably have them called overflow-vertical or overflow-horizontal instead).
  • Misc CSS cleanup and renaming of icon selectors

Checklist

  • I verify the build is in a non-broken state

  • I verify all changes are within scope of the linked issue

  • I regenerated all CSS files under dist folder

  • I tested the UI in all supported browsers

  • I did a visual regression check of the components impacted by doing a Percy build and approved the build

  • I tested the UI in dark mode and RTL mode

  • I added/updated/removed Storybook coverage as appropriate

@agliga agliga changed the title feat: changed docs to use new icons feat: changed docs to use new icons and removed old icons Mar 10, 2023
@ianmcburnie
Copy link
Contributor

So it seems like we can replace all of these icon classes like icon--article-24 now and just go with icon--24, icon--16, etc?

Screen Shot 2023-03-10 at 2 19 57 PM

@ArtBlue
Copy link
Contributor

ArtBlue commented Mar 13, 2023

So it seems like we can replace all of these icon classes like icon--article-24 now and just go with icon--24, icon--16, etc?

Unfortunately, I don't think we can because the widths of icons with the same heights can be different. The icons still need specific width and height. Sometimes, the heights and widths can be the same, but not always.

@LuLaValva
Copy link
Member

Some notes from a pass of the skin site:

  • Icons don't react to height and width defined in svg. This effects
    • Button in "Adjacent Icon Button" and "Flexible Button" examples
    • The chevron-down icon for almost every dropdown button
    • Check marks in listbox
    • Probably a few other places that I didn't notice
  • The first example in star rating doesn't work in dark mode. I don't think this is because of Andrew's changes, but it's icon related so maybe we should fix it here
  • Probably not related to icons, but "Labelling a Menu Button" in menu button also doesn't work in dark mode

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Mar 14, 2023

So this PR is on hold due to discussions around the addition of the artboard to the SVG itself. This is a BIG change. I was under the impression that artboards had not been added as no decision had been made on that from us in discussions with design team.

There are pros & cons, but ultimately artboards on icons remove a developers ability for precision layout & positioning of icons - once the artboard is there it is impossible to remove (negative margins might work - but feels hacky) - devs did not like it (including me), we learned from this in the past. We know that cropped versions work, we've had no problems with them, so what do developers gain from this?

Some options:

  1. Keep the new artboards (increases amount of breaking changes we introduce due to increased white space around icons) - lose flexibility of precision layout
  2. Same as 1, but create cropped versions on demand 3.
  3. Use artboards for majority of "square" aspect ratio icon, use cropped for non "square" icons
  4. Maintain cropped and non-cropped versions of icons
  5. Keep cropped SVGs only, and simulate the artboard with a CSS class on the SVG directly
  6. Keep cropped SVGs only, and simulate the artboard with a wrapper element and class6.

Maybe other options?

@@ -456,6 +456,7 @@ icons:
- sort-up-12
- split-view-24
- split-view-filled-24
- star-dynamic
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this icon needs to be included. It's the icon we use to create ratings dynamically. This wouldn't make sense as a standalone icon.

@agliga
Copy link
Contributor Author

agliga commented Mar 17, 2023

I think we discussed offline and seem to be okay with moving forward with this.

@agliga
Copy link
Contributor Author

agliga commented Mar 20, 2023

If there are no other major issues, this should be ready to merge.
I think any other minor changes will do up in follow up PR which will take care of program badges (see #2012)

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looking good from my end,

@agliga agliga merged commit 35caa11 into 16.0.0 Mar 20, 2023
@agliga agliga deleted the new-icons-docs branch March 20, 2023 20:35
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.

None yet

4 participants