goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.22k stars 133 forks source link

Lucide icons #147

Closed r3ps4J closed 6 months ago

r3ps4J commented 6 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Which package are you modifying?

What is the current behavior?

Currently radix icons are used for the default style.

Closes #122

What is the new behavior?

With this PR lucide icons are used for the default style.

Does this PR introduce a breaking change?

Other information

r3ps4J commented 6 months ago

Hi! Made this as a draft since it's not completely done yet. Still have to update the generations for the cli and examples in the docs. (I assume there is not a command for this that generates it based on the story book right?)

There are a couple of things I ran into:

r3ps4J commented 6 months ago

@goetzrobin got any idea about those things?

goetzrobin commented 6 months ago

Hey there! Just getting back to this now.

There are a couple of things I ran into:

Thanks so much for all your hard work!

r3ps4J commented 6 months ago

I found a better way to do the fill while still using the hlm-icon by applying the class the all children of the children of hlm-icon with *:*:fill-current. It's a bit hacky but it works, when using the ng-icon component directly it still needs a single *: anyway so I think this is a good compromise. (e189a805c27bb04383b9d57a340f7963798e52ef)

I also found out that the dropdown menu items had incorrect paddings for the icons and such so I fixed that as well in 2764d333588d9651c362c5a5a4b613f7d9e23794 and removed the unnecessary inset properties in 346594f785e5b0f49a024ce1099e66e557eec329 since that padding is always part of the classes to make space for the icons.

Removed all the unused icons I could find in 9866b5bb67520cdd561cf72f1f952703d1aad7a0. Next I'll try to fix the command prompt styling.

@goetzrobin Quick question: since there have been commits to the repo in the meantime, what is the best way for me to get those commits to my branch? Saw that there are some radix icons in the new pagination component too so might as well replace them.

r3ps4J commented 6 months ago

I managed to fix the icon for the command prompt, I was confused why it was displaying so weird but it turns out it was just a different size.

That being said, there are some margins, paddings and widths/heights that seem to be different than the default style. The entire command prompt also looks a lot smaller. Sadly I don't have time right now to go over the entire command prompt, so maybe that should get it's own issue and someone can pick it up at some point?

goetzrobin commented 6 months ago

Sounds good to me

goetzrobin commented 6 months ago

I found a better way to do the fill while still using the hlm-icon by applying the class the all children of the children of hlm-icon with *:*:fill-current. It's a bit hacky but it works, when using the ng-icon component directly it still needs a single *: anyway so I think this is a good compromise. (e189a805c27bb04383b9d57a340f7963798e52ef)

I also found out that the dropdown menu items had incorrect paddings for the icons and such so I fixed that as well in 2764d333588d9651c362c5a5a4b613f7d9e23794 and removed the unnecessary inset properties in 346594f785e5b0f49a024ce1099e66e557eec329 since that padding is always part of the classes to make space for the icons.

Removed all the unused icons I could find in 9866b5bb67520cdd561cf72f1f952703d1aad7a0. Next I'll try to fix the command prompt styling.

@goetzrobin Quick question: since there have been commits to the repo in the meantime, what is the best way for me to get those commits to my branch? Saw that there are some radix icons in the new pagination component too so might as well replace them.

You can merge the main branch into your branch or rebase.

goetzrobin commented 6 months ago

@r3ps4J let me know if you need any help with this!

r3ps4J commented 6 months ago

@r3ps4J let me know if you need any help with this!

Uhm, if you have like a list of what to copy where that would save me some time looking for it! I'll try to merge upstream asap and change those icons as well.

goetzrobin commented 6 months ago

What exactly do you mean by a list to copy? If you want I can open a PR against your branch with resolved conflicts? I am trying to get the change to the Lucide icons in before I release the next alpha so everything looks even better :)

r3ps4J commented 6 months ago

Oh no I meant where to copy it to. Like the cli generator files and docs.

I'll get the latest changes tomorrow.

goetzrobin commented 6 months ago

@r3ps4J I can take care of that as part of the release!

goetzrobin commented 6 months ago

@r3ps4J I can take care of that as part of the release!

r3ps4J commented 6 months ago

Awesome, that saves me a lot of work! I'll ping you when I got the latest changes ready.

r3ps4J commented 6 months ago

So the new select component is also completely made in the New York style rather than the default. Seems the same as the dropdown with select/radio items, which I fixed in this PR, but perhaps it's an idea that someone goes over all components and checks if they are in the default style? It's kinda mixed right now 😅. I'll make an issue for the select it since I don't really have to time to look it over now.

r3ps4J commented 6 months ago

@goetzrobin I think this should be it. I didn't remove @ng-icons/radix-icons from the main package.json since it's still used by the other packages. If you need me to change something let me know!