Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove generic selectors from rules using !important #80

Open
AfroThundr3007730 opened this issue Sep 2, 2019 · 12 comments
Open

Remove generic selectors from rules using !important #80

AfroThundr3007730 opened this issue Sep 2, 2019 · 12 comments

Comments

@AfroThundr3007730
Copy link
Member

Generally speaking, we should not be applying !important to generic selectors like div or span (the two biggest pain points in this style), as this causes cascading !important rules all over the style, creating an unmaintainable mess.

This is even more problematic when said selectors have multiple :not() pesudoclasses appended, increasing their specificity beyond the capability to be overridden.

I've already (mostly) remedied the div problem, but we still need to deal with span, among others. Even after this is fixed, the majority of the existing rules will still be using an unnecessary !important flag, and it will be difficult to weed out the cases where it's no longer needed.

Perhaps the long-term solution to this might be a rewrite of the desktop style.

@the-j0k3r
Copy link
Member

the-j0k3r commented Sep 2, 2019

We also shouldnt be using div and span as selectors unless thats also present in default style. Equally unmaintainable mess.

Im not going to get into this, I would rather re-write the style using a generator and then only doing manual rules to account for theinline styles or something that needs to be manual.

@oowekyala
Copy link
Contributor

One ugly consequence of using !important: the German Wikipedia template for color legends ("Farblegende") uses a thick border and the border color to produce a colored square. I won't comment on the implementation choice, the fact is, the following css rule overrides the border color to be gray:

div:not([style*="-color"]):not(.uls-menu), div[style*="background-color"],
span:not([class*="color"]), table:not(.hp-portalen), #talkheader td,
tr:not([style*="border-bottom"]), th, .infobox tr[style*="background-color"],
td, h1, h2, h3, h4, h5, h6, ul, li, input, select, .mw-body-content code,
.lang-list-button:hover, .oo-ui-buttonElement-button, #mwe-popups-settings,
#mwe-popups-settings header, html .thumbimage {
border-color: var(--gray-5) !important;
}

For example the legend on the first picture of this page shows as just gray squares:

Capture du 2019-11-04 18-12-57

@the-j0k3r
Copy link
Member

@oowekyala Thats actually not because of !important rule but this span:not([class*="color"]) being assigned to a border-color and because this is an inline style.

I should have a fix (exclusion) for those later

@the-j0k3r
Copy link
Member

@oowekyala upgrade to v2.3.22 it should fix the issues you reported and also fixes other issues you didn't report =)

@oowekyala
Copy link
Contributor

@the-j0k3r Oh ok my bad, sorry for cluttering this issue.

I should have a fix (exclusion) for those later

Good to know

@the-j0k3r
Copy link
Member

Its done, upgrade

@oowekyala
Copy link
Contributor

oowekyala commented Nov 4, 2019

@the-j0k3r Thanks, it works! I think though, you might have forgotten to bump the version number. I didn't get an auto update Nvm didn't look at the next commits

@the-j0k3r
Copy link
Member

the-j0k3r commented Nov 25, 2019

As I found out, simply removing the important tag isnt enough, but I have separated some of these rules already but there are other rules to be done.

72dd109

along with #79 this is coming along, but its consuming a silly amount of time I should be dedicating to other projects.

Hence I always wanted a re-write rather than botched fixes.

@AfroThundr3007730
Copy link
Member Author

If we were to go with a rewrite for the desktop style, how would we proceed?

I'm thinking what I did with the Minerva skin for mobile would be the best approach, but would be fairly time consuming to accomplish. A lot of the current rules are going to be out of place since they're overcompensating for the !important rules elsewhere. It feels like the majority of it will have to be completely from scratch (not that I'm against doing it that way).

I could start another branch to explore this. I'd most likely comment out the current rules and start writing the core styles, then begin transplanting bits that make sense to keep.

@the-j0k3r
Copy link
Member

I already said several times how best to do it, with a generator then only do manual inline styles.

@the-j0k3r
Copy link
Member

Also, with some care all existing valid manual styles can probably just be copied over. That is after generator been run.

@the-j0k3r
Copy link
Member

From tests and experience, I can say doing this will ensue a deluge of proverbial worms from opening this can. depending on what is removed.

It would probably be safe to remove standard selectors that exist in default CSS from i!mportant rules but this also depends on many other selectors that dont follow the default CSS entries also being removed from !important ,they just cause a cascading effect of (mis-)usage of i!mportant in other rules just so some properties take effect.

Overall if its not !important in default CSS I just dont add to the problem,

/me dreams of a style generator 😴 💤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants