cmseguin / rollup-plugin-react-scoped-css

A rollup plugin designed to allow scoped css to be run in react (Compatible with vite and rollup)
38 stars 6 forks source link

Improved tests for css scoping and fixed typos in readme. #51

Closed wirdehall closed 7 months ago

wirdehall commented 7 months ago

Improvement(scoped-css): Tests are now ganular and split. Improvement(scoped-css): Readme typos fixed. Bug Fix(scoped-css): Fixed edge-case with css id's.

cmseguin commented 7 months ago

That looks good thanks! :)

Everything so far is working well with my setup! I will spend more time over this week to see that we cover most edge cases but this is looking good!

cmseguin commented 7 months ago

Should be part of 1.0.0-alpha.5

cmseguin commented 7 months ago

I also cleaned the code a bit and added support for nested media queries :) 1.0.0-alpha.7

wirdehall commented 7 months ago

It's funny, I thought about this possibility and thought about converting it to a recursive function. But after googling a bit quick the top two results told me it wasn't supported in regular CSS. So I decieded against implementing it as it would effect performance a tiny bit. BUT, now I checked again and checked more than the first two answers on google. Turns out it is supported in vanilla CSS: https://caniuse.com/mdn-css_at-rules_media_nested-queries

DOH! Oh well, good catch!

wirdehall commented 7 months ago

I also thought about the fact that if we should implement the variant you implemented yesterday where all selectors get the data attribute instead of just the last one. (Like it is in Angular) but since Vue.js appeared too only scope the last selector I didn't want to change the behavior too much.

I'm glad you changed that though since it's way more accurate in doing it "the Angular way". Should have just gone with my gut feeling on both these two topics 😅

cmseguin commented 7 months ago

Haha I'm glad! :) If you see anything else feel free to open a PR to change it.

I know I didn't reply to your other comment about the data-v-{hash} I think we can remove it I don't know how I would feel to have it be data-{hash} though. I will try to add a discussion tab on the Repo so we can keep track of these ideas! :) There are other things I wanted to add from the beginning of this project like the ability to add a <style scoped>...</style> to the JSX and the styles would only be scoped to the components next and below in the tree. This would probably be a 2.0.0 though.

cmseguin commented 7 months ago

I created a discussion here: https://github.com/cmseguin/rollup-plugin-react-scoped-css/discussions/52 So we can keep track of ideas for the future.

wirdehall commented 7 months ago

Haha I'm glad! :) If you see anything else feel free to open a PR to change it.

I know I didn't reply to your other comment about the data-v-{hash} I think we can remove it I don't know how I would feel to have it be data-{hash} though. I will try to add a discussion tab on the Repo so we can keep track of these ideas! :) There are other things I wanted to add from the beginning of this project like the ability to add a <style scoped>...</style> to the JSX and the styles would only be scoped to the components next and below in the tree. This would probably be a 2.0.0 though.

Too answer you here reagarding these two things. Angular does data-{hash}, if it works for them I don't see why we would need the prefix. The odds of there ever being a data attribute with the name of the hash for the specific file is astronomical and super unlikely, it's not something a human would do. It whould have to be another repo in which case if there is a conflict, having the option of adding a prefix makes sense. But I don't think it should be there by default.

Regarding <style scoped>...</style>: I personally think this is an antipattern that should be discurraged. It's a good thing having the CSS out of the way when your working on the functionallity. I don't think I have ever worked on functionallity at the same time as the styling. So having it in the same file is just creating more clutter. There are some extreamly specific edge-cases where I might work with both at the same time but at that point it's not a big cost of having two files open. Things like animation (usually third party libs does this so good now a days that we don't have to do this in css, better even as we get transitions on removal), and also things like when adding an active class or something. But usually I just handle the styling for all the devices once everything works functionallity wise. For me atleast the CSS files tend to get quite big as we need to make the styling work for all possible devices. I wouldn't want to come to a project where more than 50% of all the lines is css in my components.

Is it something you use in your projects? I think it's a bad habbit re-introduced by Vue.js. It gives me flashbacks to the old php era around 2005-2007 where php, html, css and mysql code where in the same files. Coming into a project like that and having to start the cleanup was a nightmare. "Separation of concerns" where invented for a reason IMO 😄

Sorry for the long rant! 😅

cmseguin commented 7 months ago

Haha I would agree with you this would not be most cases for me. I would use it personally to add more dynamic css or for components that are less than 10 lines long that would need 1 or 2 lines of css. Definitely not my usual workflow but I liked the idea of giving the option. But I have to agree that it could easily get very messy so maybe it's a box that shouldn't be opened. Anyways if I am to consider this I wouldn't do it part of this 1.0.0 release anyway!