Page MenuHomePhabricator

Refactor the CSS in IPInfo [L]
Closed, ResolvedPublic

Description

@phuedx pointed out that now might be a good time to be more deliberate in our CSS usage (and use fewer magic numbers).

Use fewer magic numbers
There are a lot of magic numbers, presumably stemming from using OOUI widgets and working around those defaults. I chatted quickly with @Prtksxna who said that we should, if possible, go with OOUI defaults and if it ends up being odd, go for the defaults laid out in https://github.com/wikimedia/wikimedia-ui-base/blob/master/wikimedia-ui-base.css.

Additionally, we could be clearer about what the magic numbers, when they're being used, are doing. For instance:

.ext-ipinfo-button-collapse,
.ext-ipinfo-button-collapse .oo-ui-buttonElement-button {
	margin: 0;
	padding: 23px;
	display: block;
}

It's not apparent to me why we need a padding of 23px and not any other number. Did OOUI not give us something we need?

Some of these magic numbers could also be pulled out into variables when they're reused in other classes. Not that I know for sure, but while skimming, I notice the number 23px come back up again:

.ext-ipinfo-infobox {
	.oo-ui-panelLayout {
		padding: 0;
	}

	.ext-ipinfo-widget {
		margin-top: -23px;
		padding: 0 38px;
	}
}

Is it related to the use in .ext-ipinfo-button-collapse? And if so, can we put it in a variable so that the CSS becomes more legible and rules that are related to each other are more obvious?

For more on magic numbers:
https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants

Normalize to base

We should use variables from https://github.com/wikimedia/wikimedia-ui-base/blob/master/wikimedia-ui-base.css wherever possible.

Clearly split up widget, infobox, and popup CSS
Since we've split up the infobox and the popup, we could also probably split up their location-dependent CSS as well so it's clearer what belongs to what. afaik, the popup will never need anything that applies to ..ext-ipinfo-button-collapse,. We also have some no-js and "always loaded" CSS in init.less which as of writing has:

  • one popup-related selector that's always loaded
  • one no-js CSS selector
  • one infobox-related selector that's always loaded

It seems like init.js should only have cross-widget and no-js CSS but someone should correct me if I'm wrong. cc @Tchanders who pointed out the file to me in a recent code review 🙇


AC:

  • init.less and ipinfo.less should have clear areas of concerns
  • baseline variables should be used wherever possible
  • magic numbers should be documented or removed

Event Timeline

ARamirez_WMF renamed this task from Refactor the CSS in IPInfo to Refactor the CSS in IPInfo [L].Dec 14 2021, 5:03 PM

Change 753472 had a related patch set uploaded (by TsepoThoabala; author: TsepoThoabala):

[mediawiki/extensions/IPInfo@master] Refactor the CSS in IPInfo

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

It seems like init.js should only have cross-widget and no-js CSS but someone should correct me if I'm wrong. cc @Tchanders who pointed out the file to me in a recent code review 🙇

I think we can leave init.less as-is, since it is small (smaller than when the task was written - currently contains one rule each for the popup and infobox). We don't want separate init modules for the popup and infobox because of the cost of defining more modules. And I think separating the small file into two is probably slightly more maintenance overhead than it's worth.

Change 753472 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Refactor the CSS in IPInfo

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