Page MenuHomePhabricator

Pull out messageboxes styles into ResourceLoaderSkinModule feature
Closed, ResolvedPublic

Description

resources/src/mediawiki.skinning/legacy.less

Acceptance criteria

  • Move styles that apply to the following into a new file resources/src/mediawiki.skinning/messageboxes.less
.messagebox,
.errorbox,
.warningbox,
.successbox
  • resources/src/mediawiki.skinning/legacy.less should @import the newly created LESS file
  • A new feature is added to ResourceLoaderSkinModule which loads those styles when interface-message-box is inside the features array. Please see elements for an example of how this should work.
NOTE: Do not move the non-standard .mw-warning-with-logexcerpt

Sign off

  • Prioritize making sure the styles are consumed in Minerva and not MobileFrontend (T233160)

Event Timeline

Do we really want to go the component way? That'd be contradicting to the topic-collected modules, which make more sense to me for the core CSS architecture?
See also topic-related .mw-warning in 'content.css'

Also let's keep in mind, that those classes are not following the CSS naming structure scheme. That is part of why they live in 'legacy.less'. If moved we should invent a mw-messagebox as well.

Also let's keep in mind, that those classes are not following the CSS naming structure scheme. That is part of why they live in 'legacy.less'. If moved we should invent a mw-messagebox as well.

that would be added effort and would require deprecating the existing classes but yes we can do that.

Do we really want to go the component way? That'd be contradicting to the topic-collected modules, which make more sense to me for the core CSS architecture?

Not sure I completely understand the question, but essentially we need to provide an alternative way to add these styles so that we can deprecate and drop the legacy feature. Right now skins that don't want all the baggage of legacy have to implement their own message box styles (Minerva for example) which is not ideal as it means multiple message box definitions. I think there should be some kind of sensible default.

If you think blocking on a Vue component library is the way to go, that can certainly be how we do this. It depends on priorities
Alternatively we could provide a separate styles module e.g. mediawiki.messagebox.styles that has these and import it inside legacy.less.

Jdlrobson renamed this task from Pull out messageboxes styles to Pull out messageboxes styles into ResourceLoaderSkinModule feature.Jul 24 2020, 2:51 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: MW-1.36-release.

Is anyone available to help me get this task through for 1.36 release? It should be straightforward to add the new feature in ResourceLoaderSkinModule and that would unblock T233160.

@Ammarpad @Volker_E @Mainframe98 ?

Should we also move the following: ?
...

Isn't that effectively legacy stuff? .error is still used in parser output for things like recursion or lua errors, but I feel those should live in parser/Parsoid stylesheets instead, especially since these are used on inline <span> elements, instead of block elements.

Also: Is this a feature that should be enabled by default?

Change 674256 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Split messageboxes.less from the legacy skin feature

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

Jdlrobson updated the task description. (Show Details)

Change 674256 merged by jenkins-bot:
[mediawiki/core@master] Split messageBoxes.less from the legacy skin feature

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

Jdlrobson updated the task description. (Show Details)

THanks @Mainframe98 !

Also: Is this a feature that should be enabled by default?

Yep I think in future this will default to on. Have made a note in T89981

Isn't that effectively legacy stuff? .error is still used in parser output for things like recursion or lua errors, but I feel those should live in parser/Parsoid stylesheets instead, especially since these are used on inline <span> elements, instead of block elements.

Agreed. we will consider separately. sorry that comment was a little outdated.