Page MenuHomePhabricator

Improve scoping of CSS
Open, LowPublic13 Estimated Story Points

Description

The CSS selectors we're using in a lot of components are not specific enough. In lots of cases we're just selecting elements by their semantic classes, not using unique classes for them. This is something inherited from the Wikistats 2 prototype that I didn't change when I was implementing the actual site.

It may be worth taking a look at changing the approach with which CSS and Vue components interact:

https://vue-loader.vuejs.org/en/features/scoped-css.html

Event Timeline

fdans moved this task from Incoming to Wikistats Beta on the Analytics board.

Hey all, this task would be really interesting.
@fdans - I had a quick question, any reason why we didn't opt for pre-processors (SASS/SCSS or LESS) to handle dynamic values and variables in CSS rather can just plain CSS? I understand that this is purely opinionated and it comes down to the preference of the designer/developer but still wanted to know the motivation behind one over the other.
@mforns - Can we think of adding this as a possible sub-task for the project T189210 for GSoC 2018. What are your thoughts on this?

@sahil505
I think that would be indeed a good sub-task for T189210!
I others are not opposed, I'll make this a sub-task of the GSoC project.
@fdans, @Milimetric, @Nuria?

Ok, added it to GSoC project T189210.
Moving to mentoring column in backlog.

Sorry I missed this, I sort of wanted to tackle it myself but if @sahil505 finds it interesting, then great! Do add me to the review though, I'd like to take a look so I can understand the structure going forward.

To answer your question, @sahil505, the most basic reason is that none of us know CSS pre-processors really well, and only I know CSS well, so keeping it simple was useful. But there are CSS variables now, and when I wrote this task I was planning on factoring out common colors and similar stuff into CSS variables: https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables. It looks like IE doesn't have support for this, but we can add it with PostCSS (https://stackoverflow.com/a/46516028/180664). I don't think the rest of the team has an opinion, so @sahil505 what do you think about this approach?

That said, if you feel strongly in favor of a pre-processor, my favorite one is SASS, and I would support a refactor to move to that instead of this task - but that's entirely up to you, because it might take more time to do something like that.

@Milimetric : Thanks for your views on this. I would be very interested to work on this task as a part of my GSoC project. @mforns has already added this as one of the subtasks for the GSoC project.

I believe the approach you mentioned is great, PostCSS do offer a way to write/define CSS variables for IE. But again we do have to decide whether we want to go with a pre-processor or just with CSS variables (custom properties). I do have fair amount of experience in both so tackling this should not be that complicated of a task.

In the end opting for either one would make our CSS way more cleaner and lesser in amount. If we go with pre-processor then we don't need to worry about browser compatibility ever again and if our codebase expands then it will be easy to manage.

P.S: My favorite one is SASS as well :-]

Ok, we talked about SASS and we think it's probably better to keep it simple for now. So CSS variables and PostCSS for now is our preference. And by the way, nice to meet you and do ping me on IRC if you have any questions, I'm milimetric on there as well.

mforns raised the priority of this task from Low to Medium.
mforns set the point value for this task to 13.

Change 437387 had a related patch set uploaded (by Sahil505; owner: Sahil505):
[analytics/wikistats2@master] [WIP] Added CSS custom properties using postcss

https://gerrit.wikimedia.org/r/437387

@Milimetric @Nuria @mforns : I'm running into an issue with this task, refactoring CSS using PostCSS & CSS variables. Whenever I try to import CSS variables file or define the CSS variables at the start of App.vue file they are somehow not getting resolved, also PostCSS is not compiling them as it should. Whenever I define the CSS variables at the beginning of App.vue file as

:root {
    --primary-bg-color: #fff;
    --topic-ex-bg-color: #72777d;
    --topic-ex-color: #fff;
}

and I use these variables in the CSS of App.vue then it would work fine, but if I try to use these variables in, suppose, TopicExplorer.vue these variables don't get resolved by PostCSS to their actual values that is used by IE. So I guess it the issue is with cross component interaction of variables using PostCSS.

The case that works fine is if I write a new style tag at the beginning of CSS of every component, that is NOT SCOPED, and define my variables in that and then use these variables in that specific component. But this method leads to a lot of code repetition and I guess this is not the most efficient way to do this?

The ideal case scenario according to me should be if we include a style.css file either in main.js or App.vue and we get to use the variables all over the Vue components OR we define all the CSS variables at the start of App.vue CSS and use it all over the Vue components.

Maybe my PostCSS config is wrong or maybe I'm not importing/writing CSS in the right way. I'm unable to track the problem. I've also looked into various docs but couldn't find any viable solution to this problem.

I have uploaded the latest patch. Can you guys please guide me with where I might be going wrong? Thanks.

Milimetric removed a project: Analytics-Kanban.
Milimetric moved this task from Mentoring to Wikistats Beta on the Analytics board.

I'm looking at this again, and I think @sahil505 was exactly right - best to define the variables in some root file and use them everywhere. Side note for myself to look at https://github.com/MadLittleMods/postcss-css-variables instead of the package @sahil505 already tried, just in case it's better somehow. Will try to pick up this work as we decide what to do about the vue 3 upgrade.

Milimetric lowered the priority of this task from Medium to Low.Aug 31 2020, 5:14 PM
Milimetric moved this task from Deprioritized to Wikistats on the Analytics board.