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

refactor: Custom icons #864

Merged
merged 13 commits into from
Feb 7, 2024
Merged

refactor: Custom icons #864

merged 13 commits into from
Feb 7, 2024

Conversation

samuveth
Copy link
Contributor

@samuveth samuveth commented Feb 3, 2024

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

src/views/Space/Overview.vue Outdated Show resolved Hide resolved
@samuveth samuveth requested a review from Sekhmet February 4, 2024 06:42
@samuveth samuveth marked this pull request as ready for review February 4, 2024 06:42
vite.config.ts Outdated
@@ -45,8 +47,12 @@ export default defineConfig({
Icons({
compiler: 'vue3',
iconCustomizer(collection, icon, props) {
props.width = '20px';
props.height = '20px';
props.class = 'text-sm';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the props to text-sm instead of using a 20-pixel width and height. This change simplifies setting the icon size. However, you need to prefix ! for overrides, like !text-lg.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer we not make icons some type of global size. I'd rather overwrite width/height to be 1em and then it will just inherit text-size from its parent (basically the same as we do with currentColor).

Then when we actually use it we can use regular font-size to control it (no need to use !important). It also makes it use size you actually request (because right now to achieve 32px you need to use text-[27px] and you actually end up with 27*1.2 so 32.4). Also it feels a bit more cleaner to set text size/color properties on a instead of this component itself to me (see the diff), but it's just nitpick.

diff --git a/src/views/Home.vue b/src/views/Home.vue
index 6ba834d..0b1d1d4 100644
--- a/src/views/Home.vue
+++ b/src/views/Home.vue
@@ -73,8 +73,14 @@ const socials = [
       <div class="space-y-2">
         <div class="eyebrow">Join the community</div>
         <div class="flex space-x-2">
-          <a v-for="social in socials" :key="social.href" :href="social.href" target="_blank">
-            <component :is="social.icon" class="!text-[27px] text-[#606060] hover:text-skin-link" />
+          <a
+            v-for="social in socials"
+            :key="social.href"
+            :href="social.href"
+            target="_blank"
+            class="text-[32px] text-[#606060] hover:text-skin-link"
+          >
+            <component :is="social.icon" />
           </a>
         </div>
       </div>
diff --git a/src/views/Space/Overview.vue b/src/views/Space/Overview.vue
index 528a786..ce8a165 100644
--- a/src/views/Space/Overview.vue
+++ b/src/views/Space/Overview.vue
@@ -107,11 +107,13 @@ watchEffect(() => setTitle(props.space.name));
         </div>
         <div class="space-x-2 flex">
           <span v-for="(social, i) in socials" :key="i">
-            <a v-if="social.href" :href="social.href" target="_blank">
-              <component
-                :is="social.icon"
-                class="!text-[22px] text-[#606060] hover:text-skin-text"
-              />
+            <a
+              v-if="social.href"
+              :href="social.href"
+              target="_blank"
+              class="text-[26px] text-[#606060] hover:text-skin-text"
+            >
+              <component :is="social.icon" />
             </a>
           </span>
         </div>
diff --git a/vite.config.ts b/vite.config.ts
index 86a6891..4fb81a0 100644
--- a/vite.config.ts
+++ b/vite.config.ts
@@ -47,7 +47,8 @@ export default defineConfig({
     Icons({
       compiler: 'vue3',
       iconCustomizer(collection, icon, props) {
-        props.class = 'text-sm';
+        props.width = '1em';
+        props.height = '1em';
       },
       customCollections: {
         c: FileSystemIconLoader('./src/assets/icons', svg =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@bonustrack
Copy link
Member

bonustrack commented Feb 5, 2024

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
Copy link
Contributor Author

samuveth commented Feb 5, 2024

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
Copy link
Member

Done

@Sekhmet Sekhmet changed the title fix: Custom icons refactor: Custom icons Feb 5, 2024
src/views/Home.vue Outdated Show resolved Hide resolved
src/views/Home.vue Outdated Show resolved Hide resolved
src/views/Space/Overview.vue Outdated Show resolved Hide resolved
samuveth and others added 4 commits February 5, 2024 23:07
Co-authored-by: Wiktor Tkaczyński <wiktor.tkaczynski@gmail.com>
Co-authored-by: Wiktor Tkaczyński <wiktor.tkaczynski@gmail.com>
@samuveth samuveth requested a review from Sekhmet February 5, 2024 16:10
@Sekhmet
Copy link
Member

Sekhmet commented Feb 6, 2024

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
Copy link
Contributor Author

samuveth commented Feb 6, 2024

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

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

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.

Not sure what you mean. Right now, on master all icons have 1.2em width/height so if we only change size to 1em for c collection we don't impact those.

And now with change to 1.11em we caused all icons across the app to appear different size than they used to be (overview used to have 20px icons, now they are 18px).

As this PR is supposed to just migrate SVGs to icon we should leave unrelated changes out of it, so I'd ideally prefer we end up with one of those 3 options I mentioned (unless there is some other option that is cleaner and doesn't impact how app looks right now).

Copy link
Member

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

tACK

@samuveth samuveth merged commit 5344667 into master Feb 7, 2024
4 checks passed
@samuveth samuveth deleted the samuv/fix-icons branch February 7, 2024 03:48
Sekhmet added a commit to snapshot-labs/sx-monorepo that referenced this pull request Feb 7, 2024
* fix: Custom icons

* fix: Use link color for more visibility on light mode

* Update src/views/Home.vue

Co-authored-by: Wiktor Tkaczyński <wiktor.tkaczynski@gmail.com>

* Update src/views/Home.vue

Co-authored-by: Wiktor Tkaczyński <wiktor.tkaczynski@gmail.com>

* fix: Revert changes

* fix: Size consistency using em

* fix: Use link color

* fix: Use larger em value

* fix: Use hard coded 20px again

---------

Co-authored-by: Wiktor Tkaczyński <wiktor.tkaczynski@gmail.com>
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.

3 participants