search

mayank99 / open-props-scss

open-props as sass variables
MIT License
22 stars 2 forks source link

Add shadows mixin and function #7

Closed woodcox closed 1 year ago

woodcox commented 1 year ago

I think the mixin is still really useful as you can apply the shadow-strength and shadow-color across multiple @include apply-shadows.

I haven't added documenting the function and mixin to generateScss.mjs. Would it be better in the README.md?

mayank99 commented 1 year ago

@woodcox Let me know if you need help or if you want me to complete the remaining work in the PR myself. Excited to see this land!

woodcox commented 1 year ago

It’s ok, I want to get this done. I wanna see this land too 😀. Just had a busy week and I was a bit distracted looking at the oklch colors in Open Props and thinking through how that might work.

woodcox commented 1 year ago

I’ve made those alterations.

mayank99 commented 1 year ago

sorry, one more comment: could you switch the variable names so that $shadow-color is used in the public-facing function argument and $-shadow-color is used internally in the function? it will also require an adjustment in the value.replace

the reason is - the user shouldn't need to add an - in their function call. it's supposed to be an implementation detail solely to avoid shadowing

woodcox commented 1 year ago

Hi, I have corrected that oversight. I didn’t realise as I’ve been testing using the less verbose: shadow(1, dark);

It did give me opportunity to think about the function and I believe this amendment is beneficial because I want to change $—-shadow-color and strength back to a css variable on a regular basis but I may only want to change them to something else in a specific instance.

mayank99 commented 1 year ago

LGTM, thanks

woodcox commented 1 year ago

No probs 🥳🎉. So looking forward to using this!