Page MenuHomePhabricator

Kill badgeicons.json and use OOUI's icons-alerts.json instead
Closed, ResolvedPublic

Description

Now that OOUI's "alerts" icon pack includes all the icons we need, we can use it and get rid of the badgeicons module.

  • Replace uses of the ext.echo.badgeicons module with oojs-ui.styles.icons-alerts
  • Delete the ext.echo.badgeicons module definition in extension.json
  • Delete modules/icons/badgeicons.json and the icon files it refers to (badge.svg and tray.svg)

Requires

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

As the implementer of this hack, I fully agree with the "Ewwww". It was a hack I was pushed into doing at the time for performance reasons, because the icons we needed were scattered across many icon packs, and pulling in all of them would be wasteful. Now that OOUI's icon packs have been reorganized, the situation is hopefully less bad, so I should look at this again.

In order to get rid of badgeicons.json and use OOUI's icons, we'd also need to upstream 'double-check' icon. @Pginer-WMF I guess you are in support of this (it's then also missing in M229 and hasn't been overhauled yet)?

Volker_E updated the task description. (Show Details)

In order to get rid of badgeicons.json and use OOUI's icons, we'd also need to upstream 'double-check' icon. @Pginer-WMF I guess you are in support of this (it's then also missing in M229 and hasn't been overhauled yet)?

The double-check icon is used to communicate the action affects multiple elements, and it is used in "Mark all as done/read" buttons on the notifications page and the New filters beta on the Watchlist. So I think it is useful as its own concept, complementing the single check with a key distinction that helps anticipating the effect of the action. Similar to the done-all icon in Material design.

Regarding the visual style it needs to be adjusted, since we want it to be aligned with the single check. @Volker_E, can you tell me which is the best place to update the icon? Since we are in the middle of the icon update process I want to avoid adding it to an outdated place.

@Pginer-WMF The best place would right now still be M229. It's not perfect from a process perspective coming in later, but it doesn't hurt either to add it there to move forward.

@Pginer-WMF The best place would right now still be M229. It's not perfect from a process perspective coming in later, but it doesn't hurt either to add it there to move forward.

I added an icon for "mark all as done" in the design source file for M229 (it will become visible when updated). That should make it ready to be available in the icon repo at some point. Let me know if anything else is needed.

I don't think that T160690: Be able to request a custom icon pack on-the-fly without bloating RL module registry is strictly required for this, but it would be a performance improvement if it ever happens.

Upstream 'double-check' icon to OOUI 'interactions' pack

I assume this is 'checkAll', and therefore done

Upstream 'double-check' icon to OOUI 'interactions' pack

I assume this is 'checkAll', and therefore done

Yes, indeed!

That's not on OOUI any more, is ready to be done in Notifications.

@Catrope will ask @Mooeypoo if this could be a good GSoC project. Staying in "To Triage" for now until that conversation.

kostajh moved this task from Needs Discussion/Analysis to Upcoming Work on the Growth-Team board.
kostajh added a subscriber: kostajh.

We'll get to this at some point in Q2/Q3/Q4 but in the meantime, volunteer contributions are very welcome!

Hi @kostajh @Catrope, I will like to work on this issue.
I am a bit unclear as what needs to be done for the first point Replace uses of the ext.echo.badgeicons module with oojs-ui.styles.icons-alerts.
On using grep I can only find two places where this module has been used i.e. in /includes/EchoHooks.php and /includes/special/SpecialNotifications.php.
Do I need to change only those statements for this task?

Do I need to change only those statements for this task?

Yes, that sounds right.

Hi @kostajh, on deleting tray.svgand bell.svg the tray and bell icon from the header also disappears as they are used in /modules/nojs/mw.echo.badge.less. So, should we delete them?

Change 504404 had a related patch set uploaded (by Kosta Harlan; owner: shivanshbindal9):
[mediawiki/extensions/Echo@master] add ooui icons

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

Hi @kostajh @Volker_E @Esanders, on deleting tray.svg and bell.svg the tray and bell icon from the header also disappears as they are used in /modules/nojs/mw.echo.badge.less. So, should we delete them and move it in some other folder or is there some other way around?

Change 510211 had a related patch set uploaded (by Shivanshbindal9; owner: shivanshbindal9):
[mediawiki/extensions/Echo@master] add ooui icons and deletes tray.svg and bell.svg from modules

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

Change 504404 abandoned by Kosta Harlan:
add ooui icons

Reason:
Abandoning in favor of I8218530ed2cdd2d81c1fc24509f36ea2b6742bd9

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

Change 510211 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Remove badgeicon module, use OOUI icon instead.

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

matmarex added a subscriber: matmarex.

I assume this is fixed now. Thank you for working on it!