Page MenuHomePhabricator

ResourceLoaderImage module should support a default color
Closed, ResolvedPublic

Description

Note: High priority as we are currently hacking around this inside Minerva. We should look to fix that ASAP.

ResourceLoaderImageModule powerfully allows setting the color of an icon via the ResourceLoader module definition. This is powerful as it lets you vary an icons color e.g. destructive; constructive; black; inverted

However, if you only need an icon in one of the variant forms, you end up with 2 definitions in the CSS - one for black and one for the variant.

In Minerva we have various icons which are only needed in the constructive form, but there is no way to ship the icon css without the black version which bloats the CSS unnecessarily.

In this example, 4 css rules are created when only 2 identical CSS rules are needed (the standard color):

		"skins.minerva.content.styles.images": {
			"class": "ResourceLoaderImageModule",
			"selectorWithoutVariant": "a.{name}",
			"selectorWithVariant": "a.{name}",
			"variants": {
				"standard": {
					"color": "#36c",
					"global": true
				}
			},
			"images": {
				"external": {
					"file": {
						"ltr": "resources/skins.minerva.content.styles.images/link-external-ltr.svg",
						"rtl": "resources/skins.minerva.content.styles.images/link-external-rtl.svg"
					}
				},
				"extiw": {
					"file": {
						"ltr": "resources/skins.minerva.content.styles.images/link-internal-ltr.svg",
						"rtl": "resources/skins.minerva.content.styles.images/link-internal-rtl.svg"
					}
				}
			}
		},

Proposed:

It should be possible to either

  1. define defaultColor on a ResourceLoaderImage module definition (The default icon takes this color.)
  2. disable any default definitions.
  3. Be smart enough to know when selectorWithVariant and selectorWithoutVariant are identical or selector field is used only to generate one css rule.

https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/skin.json#L178

Let's go with option 1.
This will require a change to the ResourceLoaderImageModule in core.

Acceptance criteria

Event Timeline

Restricted Application added a project: Performance-Team. · View Herald TranscriptJun 21 2018, 10:04 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 441421 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Update: link icons for standard density displays and CSS

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

Change 441421 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Update: link icons for standard density displays and CSS

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

Be smart enough to know when selectorWithVariant and selectorWithoutVariant are identical or selector field is used only to generate one css rule.

I don't like this, it works for the particular case here, but what if there is more than one variant? (We should not have allowed selectorWithVariant and selectorWithoutVariant that are identical and with no {variant} variable, but I did not think to add a check for that, and doing it now would break hacks like yours ;) )

I think we need a separate option like defaultColor.

@Jdlrobson @Niedzielski Would you like to implement it? I guess you just need to add handling for it to the ResourceLoaderImageModule constructor, pass it to ResourceLoaderImage in ResourceLoaderImageModule::getImages(), and make ResourceLoaderImage::getImageData() call variantize() if it's set.

Jdlrobson triaged this task as High priority.Jun 22 2018, 12:07 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: MinervaNeue.
Jdlrobson updated the task description. (Show Details)

I'll see what I can do :)

Change 441996 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] ResourceLoaderImage module definitions can define a defaultColor

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

Change 442225 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] External link icons should use defaultColor

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

As discussed in latest FESG meeting: T198339

Change 442225 had a related patch set uploaded (by Bartosz Dziewoński; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] External link icons should use defaultColor

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

Change 441996 merged by Jdlrobson:
[mediawiki/core@master] ResourceLoaderImage module definitions can define a defaultColor

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

Jdlrobson closed this task as Resolved.Jun 27 2018, 9:22 PM
Jdlrobson claimed this task.
Jdlrobson added subscribers: pmiazga, Jdrewniak.

FYI @Niedzielski @Jdrewniak @pmiazga in case you are working with icons in future!

Change 442225 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] External link icons should use defaultColor

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

Vvjjkkii renamed this task from ResourceLoaderImage module should support a default color to uhaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed Jdlrobson as the assignee of this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from uhaaaaaaaa to ResourceLoaderImage module should support a default color.Jul 2 2018, 11:07 AM
CommunityTechBot closed this task as Resolved.
CommunityTechBot assigned this task to Jdlrobson.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.