search

Fermain / -mollify

9 stars 9 forks source link

Add prop "variant" to Icon.svelte #174

Closed pretzL closed 9 months ago

pretzL commented 9 months ago

Our current Icon component only takes in a prop name. It could also take in a prop variant for deciding if the icon should be filled or outlined, which Material Icons offers. Research should be made to see what most icons currently use, which should be set as the default value for variant.

MegumiKim commented 9 months ago

@pretzL I assigned myself on this task. Just want to double check what do you mean by "Research should be made to see what most icons currently use".

pretzL commented 9 months ago

@pretzL I assigned myself on this task. Just want to double check what do you mean by "Research should be made to see what most icons currently use".

I mean for the default setting. When you define the let variant you should also define the type and a default value. That default value should correspond to either "filled" or "outlined" as per material-icons default spec. Which of the two should be decided based on what icon type we've used the most in the project. @Christonn93 removed the icon-f and icon-o classes so you'd need to go back before that commit to see where and how many times each has been used I guess.

MegumiKim commented 9 months ago

@pretzL It seems all icons are originally (before Chris's PR) filled variant (icon-f, there was no icon-o as far as I could see).

We can also chose to use only default/filled variant. That would also reduce building size: (https://www.npmjs.com/package/material-icons#reducing-build-size)

What do you say?

pretzL commented 9 months ago

@pretzL It seems all icons are originally (before Chris's PR) filled variant (icon-f, there was no icon-o as far as I could see).

We can also chose to use only default/filled variant. That would also reduce building size: (https://www.npmjs.com/package/material-icons#reducing-build-size)

What do you say?

In some cases it's good to have options, but it's really not that necessary. We'll likely always use the same icon style throughout the project, so it's not necessary to add a variant if we want to reduce the build size by only importing the filled stylesheet. What say you @Fermain ?

Fermain commented 9 months ago

This project is incredibly bloated, because we haven't turned attention to optimisation yet. I favour options over build size at this stage in the game.