influxdata / clockface

UI Kit for building Chronograf
https://influxdata.github.io/clockface
MIT License
44 stars 18 forks source link

Refactor Icon Set #797

Closed brandenTenbrink closed 2 years ago

brandenTenbrink commented 2 years ago

Closes #

Changes

Replaced the old icon set with the set that matches Marketing's request. Unused icons have been removed. All icons have been updated to use Remix Icon.

Screenshots

// Add screenshots here if relevant

Checklist

Check all that apply

ChitlangeSahas commented 2 years ago

Let me know what you think of this:

I see the intent of wanting to remove the unused icons. I would totally be on board with it if it was in the UI repo but since these icons live in the Clockface repo and we consider clockface as an open source repository for other people, removing these makes clockface tightly coupled with our UI repo.

If we are wanting to head in the direction where we consider clockface as something that just hosts UI components, maybe we should have a broader discussion with the UI team and come to a conclusion on that.

The second concern I have is that I assume someone put a bunch of effort at some point to pick out these icons so that they go with each other, removing them means we would have to start fresh when picking any new icon for a feature in the future and that might mean more bothers for the design team whenever a new icon is needed. Something to consider :)

taramk commented 2 years ago

@ChitlangeSahas Re: maintenance concerns—this was one of our concerns as well. Previously we had a mix of icons from multiple libraries and this update is meant to improve consistency by using only the Remix icons library, with custom icons only when absolutely necessary. Remix is a large library and so it should be pretty future-proof. Probably 2/3 of our icons are already using Remix.

After some internal discussion as a design team, we feel that our first priority is to get Clockface to a good place for internal use before we focus on making it useful to the public. Right now it's somewhat inconsistent and incomplete. My suggestion for Clockface (for now) would be to have documentation recommending people use Remix icons if they need icons that aren't in our set.

cc @mavarius

ChitlangeSahas commented 2 years ago

Sounds good @taramk, thank you for the clarification 👍 Good to know that the icons are from remix, helpful for the future.

I was curious about one more thing, @brandenTenbrink , what process did you use to know what icons were unused? Did you check it with the UI codebase because we use clockface IconFont in there like : IconFont.Info_New and I see that you're removing it from the code as of this PR.

brandenTenbrink commented 2 years ago

Sounds good @taramk, thank you for the clarification 👍 Good to know that the icons are from remix, helpful for the future.

I was curious about one more thing, @brandenTenbrink , what process did you use to know what icons were unused? Did you check it with the UI codebase because we use clockface IconFont in there like : IconFont.Info_New and I see that you're removing it from the code as of this PR.

Icons were checked in different repos, but some may have been missed. I know there is some manual update to be done--referenced here: https://github.com/influxdata/ui/issues/4976

But if there are more, then we can add them to the issue to be updated to as well.

ChitlangeSahas commented 2 years ago

Offline: Branden has verified all the used icons in the UI and made sure that they are in the clockface library.