code4rena-dev / components-library

Components library for Code4rena, and accessible as a package to be used in React based Code4rena products.
https://components-library-wine.vercel.app
0 stars 0 forks source link

Add new icons #71

Closed captainmangoC4 closed 3 months ago

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
components-library ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 1, 2024 4:48pm
SamusElderg commented 3 months ago

hahah very sneaky adding a few more i see 😂 ill check them, actually i have a 'temporary' icon component in the .com repo too with notes to 'move them to component library' ill see if they are an easy add too, maybe not as i dont like the small/medium/large implementation and went for a integer (pixels) input for those i think

SamusElderg commented 3 months ago

all good! tested the dot/suggestion ones too!

captainmangoC4 commented 3 months ago

hahah very sneaky adding a few more i see 😂

lol, I didn't expect you to review so soon! Apologies I added even more! I thought you'd still be sleeping and didn't realize you were already reviewing.

captainmangoC4 commented 3 months ago

i dont like the small/medium/large implementation and went for a integer (pixels) input for those i think

We could change it to allow either s/m/l or pixels. We could just default to size in that switch statement where it translates size to pixels.

SamusElderg commented 3 months ago

i dont like the small/medium/large implementation and went for a integer (pixels) input for those i think

We could change it to allow either s/m/l or pixels. We could just default to size in that switch statement where it translates size to pixels.

Yeah i think an improvement would be to have the existing 3 string sizes and then optionally an integer to override it with a custom size, good idea! I just had a look at the code and the string is just translated to a pixel size anyway, so should be an easy change (we dont need to do it now though no immediate need)

Also i had a quick look at the casing of the properties are realised a little 'gotcha' i missed; react doesnt play well with non camelCase props even when they are inside a child svg.

So we will need to change anything like fill-rule and clip-rule into fillRule and clipRule to ensure once we are consuming the icons we dont have a fun amount of yelling in the console. I think ill make an eslint rule for this at some point as its a super easy one to overlook/forget

Ill make a list of search terms to search-replace below and edit if i find more to make the change easier when you get to it:

Note: i finished checking its literally just those two that need to be changed (ping me if you want me to change them and commit to your pr)

captainmangoC4 commented 3 months ago

Good catch! I'll fix those now; thanks Samus!