Page MenuHomePhabricator

Re-introduce a core icon pack with the icons necessary to render the built-in widgets
Closed, DeclinedPublic

Description

We've recently removed the core icon pack in OOjs UI (https://gerrit.wikimedia.org/r/#/c/238324/) and we're in the process of updating everything not to rely on it (T166730). But I just realized that we've made a huge omission – no one looked at which icons we use on OOjs UI code ourselves. I have made this analysis now: here's which icon packs are required by the four parts of the library:

  • core: content, alerts, interactions
    • FieldLayout uses 'info' (if 'help' or 'notices' config option is given)
    • FieldLayout uses 'alert' (if 'errors' config option is given)
    • FieldsetLayout uses 'info' (if 'help' config option is given)
    • PopupWidget uses 'close' (if 'head' config option is given)
    • TextInputWidget uses 'search' (when 'type' is 'search', deprecated)
    • SearchInputWidget uses 'search'
  • widgets: interactions, content, editing-advanced, movement, moderation
    • NumberInputWidget uses 'add', 'subtract'
    • SearchWidget uses 'search'
    • SelectFileWidget uses 'upload', 'attachment', 'close'
    • OutlineControlsWidget uses 'add', 'collapse', 'expand', 'trash'
  • toolbars: movement
    • ListToolGroup uses 'collapse', 'expand'
  • windows: movement
    • ProcessDialog uses 'previous' (in mobile mode, if it has a 'back' action)

Counted together, the now-required icon packs (alerts, interactions, content, editing-advanced, movement, moderation) are far larger than the old core pack. Even if we look at the core widgets/layouts only (content, alerts, interactions), the newly required packs are still larger than the old one. So it looks like we've made a huge mistake. (As a side note, it seems that the grouping of icons into packs does not reflect the use cases very well.)

I think we need to re-introduce a core icon pack with the icons necessary to render the built-in widgets. I don't think we can require our users to keep track of this bookkeeping to add the 'interactions' pack or whatever every time they use a PopupWidget (or use another widget that uses PopupWidget…).

Event Timeline

matmarex created this task.Jun 3 2017, 12:11 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 3 2017, 12:11 PM

Change 357011 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Add appropriate OOjs UI icon pack dependencies for OOjs UI itself

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

matmarex triaged this task as Unbreak Now! priority.Jun 3 2017, 12:16 PM
matmarex added subscribers: Volker_E, Jdforrester-WMF.
Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptJun 3 2017, 12:16 PM

I propose declining this.

“far larger“ is fairly strong words. What size increase do we talk about gzipped?

Change 357011 merged by jenkins-bot:
[mediawiki/core@master] Add appropriate OOjs UI icon pack dependencies for OOjs UI itself

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

demon added a subscriber: demon.Jun 27 2017, 7:34 PM

This has been open almost a month, and the only patch was merged. Is this still UBN? What needs to be done?

Jdforrester-WMF closed this task as Declined.Jun 27 2017, 7:44 PM

It's Volker's call as PO. UBN for the project, but it has no impact in production.

Restricted Application removed a subscriber: Liuxinyu970226. · View Herald TranscriptJun 27 2017, 7:44 PM