snapshot-labs / sx-ui

An open source interface for Snapshot X protocol.
https://snapshotx.xyz
MIT License
34 stars 29 forks source link

refactor: Custom icons #864

Closed samuveth closed 8 months ago

samuveth commented 8 months ago

Summary

  1. Refactor custom icons to use unplugin-icons, for example, <IC-icon-file-name />.
  2. Remove the fill attribute from the icon SVGs to allow setting the color with text-color.
  3. Relocate icons to a new folder assets/icons.
  4. Add hover effects and refactor social icons on the Home and Overview pages, maintaining the same hex color value. Transition to a color variable will occur post-integration of the design system from Figma. image

How to test

  1. Go to Overview and Home page
bonustrack commented 8 months ago

The size of the icons are slightly bigger than, is it possible to get it same as before?

Also somehow menu item are missing:

Here is see treasury page as menu item https://snapshotx.xyz/#/sn:0x0041ada9121061198b52ae28edeec8ace7c23f2ba8d66938c129af1a2245701c

Here I don't see it (this PR) https://bafybeialjdiigbro6womclncube2mjx3qptllqqxlwymylopv7utun7cni.on.fleek.co/#/sn:0x0041ada9121061198b52ae28edeec8ace7c23f2ba8d66938c129af1a2245701c

I don't see the icons to cast vote also in the feed.

samuveth commented 8 months ago

The size of the icons are slightly bigger than, is it possible to get it same as before?

It's only for the social icons, the others are the same. I could use text-[26.6px] but I don't think it really matters that it's 1px bigger than before.

Also somehow menu item are missing:

The branch wasn't up to date, it should work now. Can you add this setting so that it's visible on PR page that the branch needs to be updated? It's in Setting > General when you scroll down

image
bonustrack commented 8 months ago

Done

Sekhmet commented 8 months ago

Nice, yeah that's cleaner! But now all the other icons are 18px instead of 20px, the only solution I found for this is changing the global text size in App.vue from base to md. Or we could add text-md to each icon.

I guess we could workaround this that way, sucks a bit because we are losing consistency. 1.2em seems good when inlining icons along text, but then it becomes troublesome when we actually want to use icon by itself and have good control of its size.

diff --git a/vite.config.ts b/vite.config.ts
index 4fb81a0..384f87c 100644
--- a/vite.config.ts
+++ b/vite.config.ts
@@ -47,8 +47,10 @@ export default defineConfig({
     Icons({
       compiler: 'vue3',
       iconCustomizer(collection, icon, props) {
-        props.width = '1em';
-        props.height = '1em';
+        const size = collection === 'c' ? '1em' : '1.2em';
+
+        props.width = size;
+        props.height = size;
       },
       customCollections: {
         c: FileSystemIconLoader('./src/assets/icons', svg =>

So I think "clean" options for now are:

  1. Diverge how those icons are used, and use 1em for custom icons.
  2. Make it use 1.2em everywhere and update sizes for our custom icons so they match up with intended sizes.
  3. Make everything use 1em and update other icons usage (but this will be lots of changes so I'd suggest starting with 1 and then doing 3 in separate PR).

Honestly I'm not sure what I'd prefer, not big fan of 2 as it's too magical and gets into your way when using standalone icons. Maybe 1 as a start and then coming back to do 3 (maybe with helper class inline-icon that just bumps size to 1.2em).

I will let you decide :D

samuveth commented 8 months ago

Doing const size = collection === 'c' ? '1em' : '1.2em'; doesn't work well either because the globe icon on Overview page is not from c collection, so would have to be seized separately again. I decided to style them all 1.11em which makes the icons closest to 20px I could get. We can change this to 1em in another PR where we refactor the icons that need it to use text-md