Uses CSS module instead of Stitches for Loader component #2178
Conversation
Signed-off-by: Mihovil Ilakovac <[email protected]>
| @@ -0,0 +1,37 @@ | |||
| .loader { | |||
| color: var(--loader-color, #1a202c); | |||
There was a problem hiding this comment.
Wrote it like this to enable users to adjust the loader color with CSS vars. It's undocumented but we can tell users that info if they ask.
There was a problem hiding this comment.
So this is still local CSS, is it? Not global, right? Because of the ".module.css"? WHo is enabling this, Vite, or are we using something special?
There was a problem hiding this comment.
Vite supports CSS modules out of the box: https://vitejs.dev/config/shared-options.html#css-modules
Signed-off-by: Mihovil Ilakovac <[email protected]>
There was a problem hiding this comment.
Left a couple of comments but approved! Btw i believe i left somewhere in the codebase a comment/TODO saying that we will have this issue with stitches and loader - can you please check it and remove it if you find it? If not then it was probably just in that post-PR review I did.
| @@ -0,0 +1,37 @@ | |||
| .loader { | |||
| color: var(--loader-color, #1a202c); | |||
There was a problem hiding this comment.
So this is still local CSS, is it? Not global, right? Because of the ".module.css"? WHo is enabling this, Vite, or are we using something special?
| export function Loader() { | ||
| const loaderClassName = ['loader', styles.loader].join(' ') |
There was a problem hiding this comment.
I would maybe just inline this, but do as you like.
There was a problem hiding this comment.
Btw why do you also need to set 'loader' class here? I would imagine only styles.loader is enough?
There was a problem hiding this comment.
I would maybe just inline this, but do as you like.
I prefer this 🤷
I would imagine only styles.loader is enough?
I wanted to give users a stable class name (styles.loader gives you a mangled name) if they wanted to style the loader in some way.
There was a problem hiding this comment.
In that case, shouldn't we name it sometihng like wasp-loader? Because this is now global CSS class if I am right -> calling it just loader seems problematic.
There was a problem hiding this comment.
Most people use Tailwind these days or one of the CSS-in-JS options, so I don't think there will be a lot of interference with built in classes. If it happens to happen, let's change it then.
There was a problem hiding this comment.
So you are saying that our global CSS hopefully wont cause any issues because others might not use global CSS themselves? That is ok, but we can't count on that. Also, as a library/framework, we should at least try to not affect their global space if we can. I think we should do this now, it is an easy change and is a good practice and we know it has a potential to cause problems down the road: it would be downright sloppy to leave this as it is now.
There was a problem hiding this comment.
Sure, I can add the wasp- prefix in a new PR.
|
@Martinsos the comment about Stitches and loader is in your post-PR review, not in an issue, so there was nothing to delete. |
Loadershouldn't depend onstitchesbeing available since it's installed only when auth is used.Moves
Loaderto use CSS Modules instead.