Electricitymap-contrib: Remove all SCSS files in favor of component styles

[fbarl]

As a part of the effort to make components more reusable (see #1681), we should consider tying the CSS styles to the components instead of keeping (all of) them in global stylesheets.

styled-components do this job pretty well in my experience.

[corradio]

My experience is that styled-components doesn't add too much compared to
directly embedding styles as Javascript Objects.
What's your recommendation?

Oli

On Tue, Mar 3, 2020 at 5:55 PM Filip Barl [e-mail] wrote:

As a part of the effort to make components more reusable (see #1681
https://github.com/tmrowco/electricitymap-contrib/issues/1681), we
should consider tying the CSS styles to the components instead of keeping
(all of) them in global stylesheets.

styled-components https://styled-components.com/ do this job pretty
well in my experience.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/tmrowco/electricitymap-contrib/issues/2261?email_source=notifications&email_token=AAMUIKDYR774WZWBJ5IS5LLRFUY6TA5CNFSM4LAOVQM2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ISC23ZA,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAMUIKHTO6RSNLZ5LQBQDWDRFUY6TANCNFSM4LAOVQMQ
.

--

Olivier Corradi

Founder, CEO

https://www.linkedin.com/in/oliviercorradi https://twitter.com/corradio

tmrow.com

[fbarl]

My experience is that styled-components doesn't add too much compared to
directly embedding styles as Javascript Objects.

I think I would generally agree on this - the only reason why I'd slightly prefer styled components is because they still keep a certain degree of separation between the looks and the logic :)

I guess I was trying to make a more general point with this issue and that is that it would be good to bring component styles into components and styled-components is just one tool to help with achieving that. I'll rename the issue.

[corradio]

Fully agreed!

[corradio]

@fbarl should we close this one? I feel there's a lack of actionability with this issue.

[fbarl]

should we close this one? I feel there's a lack of actionability with this issue.

I think we could rename the issue to _Remove all SCSS files in favor of component styles_ which would make it more actionable - do you think that would be a worthwhile effort? It would make the whole codebase a bit more modular IMO.

[corradio]

Let's do that! It's also a great call to action for contributors.

[pierresegonne]

I think the task might seem a bit daunting to contributors,

Could we provide an example of refactoring to make it clearer, as well maybe as a starting list of concerned components?

[fbarl]

I think the task might seem a bit daunting to contributors

The task is big but not very technically challenging, so contributors with some past experience with styled components shouldn't encounter many difficulties giving it a go, even if they are not very familiar with our JS codebase - that was my thinking for sticking the help wanted tag although I admit we might have better luck if we try splitting it into smaller issues :slightly_smiling_face:

Could we provide an example of refactoring to make it clearer...

Sure! So the idea is that /web/src/scss currently contains a lot of styles that cross reference different parts of the DOM, making it hard to:

1. Find all the styles that are being applied to the component of interest, as they might be scattered across different files
2. Make style changes to a particular component while making sure nothing else in the app gets affected
3. Use a theme system without reloading the stylesheet files or the whole app

So the objective would be to get rid of all those files completely, replacing them with:

1. Styled components, i.e. nodes with explicitly tied in styles that live in component JS files - see InfoTooltip and Toggle as examples of fully transitioned components (all their styles live in the same files as their React definitions)
2. Global styles like in globalstyle.js if they need to be applied beyond the reachable React DOM context e.g. on the body or to modify the styles of some external dependencies

... as well maybe as a starting list of concerned components?

styles.scss already has a rough list of components that are still not fully transitioned, so I'd suggest taking these one-by-one (perhaps from the bottom up) to complete the task:

@import "variables";
@import "mixins";
@import "base";

@import "layout/header";
@import "layout/footer";
@import "layout/left-panel";

@import "content/map/map";
@import "content/map/controls";
@import "content/map/legend";
@import "content/panel/country-panel";
@import "content/panel/time-slider";
@import "content/panel/zone-list";

@import "components/faqs";
@import "components/messaging";
@import "components/modal";
@import "components/meta";

It _is_ a lot of work, so maybe it would make sense to split this issue into multiple smaller issues, I'd let @Kongkille and @FelixDQ decide how to proceed here :wink:

[thumbsupep]

I'd like to help with this. maybe header.js https://github.com/tmrowco/electricitymap-contrib/blob/master/web/src/layout/header.js#L4?

[fbarl]

Happy to hear you'd like to help @thumbsupep!

Sure you can start with the header - feel free to open a draft PR from your first commit so that we can follow your work and help you out if needed :slightly_smiling_face:

[thumbsupep]

@fbarl I created a draft PR, but I have a question! Where is the best place to put the variables and breakpoints? I created a theme and a theme.js file (not sure which folder it should go in), but I'm not sure if you already have a plan for variables in styled components that I looked over.

[fbarl]

That's great @thumbsupep! I took a look at your PR and left some comments there - it's easier to keep an async discussion there than extending on a long thread here :wink: