Page MenuHomePhabricator
Create Task
Maniphest T345653

Follow up work: Clean up icon classes in Popups
Closed, ResolvedPublic1 Estimated Story Points
Actions

Assigned To
None
Authored By
bwang
Sep 5 2023, 5:59 PM
Tags
Referenced Files
F37748563: screenshot 81.png
Sep 22 2023, 2:31 AM
F37748561: screenshot 80.png
Sep 22 2023, 2:31 AM
F37724136: Screenshot 2023-09-14 at 3.45.43 PM.png
Sep 14 2023, 8:47 PM
F37719694: screenshot 33.png
Sep 14 2023, 2:33 AM
Subscribers
Aklapper
bwang
Edtadros
Jdlrobson

Description

Description

We have removed core icon classes from Popups, but some of the icon classes still reference 'mw-ui-icon' confusingly.

TODO

  • Remove 'mw-ui-icon' prefix in favor of 'popups-icon'
  • Remove any leftover selectors from the old icon classes (i.e. icon selectors using :before)

Sign off

https://phabricator.wikimedia.org/T345653#9165402
https://phabricator.wikimedia.org/T345653#9189649

QA Results - Beta

ACStatusDetails
1✅T345653#9165402

QA Results - Prod

ACStatusDetails
1✅T345653#9189649
2✅T345653#9189649

Details

SubjectRepoBranchLines +/-
Remove unhelpful commentmediawiki/extensions/Popupsmaster+0 -2
Rename icon classes in Popups to use popups-icon prefix instead of mw-ui-icon, replace resource loader icons with codexmediawiki/extensions/Popupsmaster+63 -162
Customize query in gerrit

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedBUG REPORTovasilevaT330570 Page previews: Last line is cropped at the bottom
 ResolvedNoneT345653 Follow up work: Clean up icon classes in Popups

Event Timeline

bwang created this task.Sep 5 2023, 5:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 5 2023, 5:59 PM
gerritbot added a comment.Sep 5 2023, 6:02 PM

Change 954312 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/extensions/Popups@master] Rename icon classes in Popups to use popups-icon prefix instead of mw-ui-icon

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

gerritbot added a project: Patch-For-Review.Sep 5 2023, 6:02 PM
bwang updated the task description. (Show Details)Sep 5 2023, 6:03 PM
bwang moved this task from Incoming to Web Team FY2023-24 Q1 Sprint 6 on the Web-Team-Backlog board.Sep 6 2023, 6:43 PM
bwang edited projects, added Web-Team-Backlog (Web Team FY2023-24 Q1 Sprint 6); removed Web-Team-Backlog.
Jdlrobson renamed this task from Clean up icon classes in Popups to Follow up work: Clean up icon classes in Popups.Sep 7 2023, 5:06 PM
ovasileva triaged this task as Medium priority.Sep 7 2023, 5:06 PM
ovasileva added a parent task: T330570: Page previews: Last line is cropped at the bottom.
ovasileva set the point value for this task to 1.
Jdlrobson mentioned this in T341899: Popups should use Codex markup instead of mw-ui-icon markup.Sep 8 2023, 6:30 PM
Jdlrobson updated the task description. (Show Details)
ovasileva moved this task from Incoming to Ready for Development on the Web-Team-Backlog (Web Team FY2023-24 Q1 Sprint 6) board.Sep 11 2023, 2:09 PM
bwang moved this task from Ready for Development to Code Review on the Web-Team-Backlog (Web Team FY2023-24 Q1 Sprint 6) board.Sep 12 2023, 3:55 PM
Jdlrobson claimed this task.Sep 12 2023, 4:43 PM
Jdlrobson reassigned this task from Jdlrobson to Edtadros.Sep 13 2023, 11:53 PM
Jdlrobson moved this task from Code Review to QA on the Web-Team-Backlog (Web Team FY2023-24 Q1 Sprint 6) board.
Jdlrobson added a subscriber: Jdlrobson.
gerritbot added a comment.Sep 13 2023, 11:58 PM

Change 954312 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Rename icon classes in Popups to use popups-icon prefix instead of mw-ui-icon, replace resource loader icons with codex

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

Maintenance_bot removed a project: Patch-For-Review.Sep 14 2023, 12:10 AM
ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.27; 2023-09-19).Sep 14 2023, 1:00 AM
Edtadros reassigned this task from Edtadros to Jdlrobson.Sep 14 2023, 2:33 AM
Edtadros added a subscriber: Edtadros.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Ventura
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: 'mw-ui-icon' prefix in favor of 'popups-icon'

@Jdlrobson, this seems to satisfy the first TODO, but I'm unsure what to do about the second TODO. If this is sufficient it passes QA in beta.

screenshot 33.png (1×1 px, 454 KB)

Jdlrobson added a comment.Sep 14 2023, 4:50 PM

@bwang I was wondering if this code can be cleaned up - do we still need to define the size? Can we at least remove the reference to mw-ui-icon-large?

	// Icon is larger than mw-ui-icon-large
	// Thus we hack it to get it to the size we want.
	.popups-icon::before,
	.popups-icon {
		background-size: contain;
		width: @width-popups-settings-help-icon;
		max-width: none;
		height: @height-popups-settings-help-icon;
		margin: 0;
		padding: 0;
	}
Jdlrobson reassigned this task from Jdlrobson to bwang.Sep 14 2023, 4:50 PM
Jdlrobson updated the task description. (Show Details)
bwang added a comment.Sep 14 2023, 8:47 PM
In T345653#9167573, @Jdlrobson wrote:

@bwang I was wondering if this code can be cleaned up - do we still need to define the size? Can we at least remove the reference to mw-ui-icon-large?

	// Icon is larger than mw-ui-icon-large
	// Thus we hack it to get it to the size we want.
	.popups-icon::before,
	.popups-icon {
		background-size: contain;
		width: @width-popups-settings-help-icon;
		max-width: none;
		height: @height-popups-settings-help-icon;
		margin: 0;
		padding: 0;
	}

That has to do with this large image

Screenshot 2023-09-14 at 3.45.43 PM.png (998×2 px, 334 KB)
that shows up inside the settings dialog. Its normally set to display none, so im not sure what its for. I can remove the reference to mw-ui-icon-large, but id have to remove this image to get rid of all the styles here. do you know if we need to keep it?

gerritbot added a comment.Sep 14 2023, 9:45 PM

Change 957828 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] Remove unhelpful comment

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

gerritbot added a project: Patch-For-Review.Sep 14 2023, 9:45 PM
Jdlrobson moved this task from QA to QA in Production on the Web-Team-Backlog (Web Team FY2023-24 Q1 Sprint 6) board.Sep 14 2023, 9:45 PM

Ahh I see. Let's remove that confusing comment. https://gerrit.wikimedia.org/r/957828

gerritbot added a comment.Sep 14 2023, 10:12 PM

Change 957828 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Remove unhelpful comment

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

Maintenance_bot removed a project: Patch-For-Review.Sep 14 2023, 10:30 PM
Jdlrobson reassigned this task from bwang to Edtadros.Sep 18 2023, 5:55 PM
Jdlrobson lowered the priority of this task from Medium to Low.
Jdlrobson mentioned this in T346851: Verify 1.41.0-wmf.30 deployment.Sep 19 2023, 11:25 PM
Edtadros closed this task as Resolved.Sep 22 2023, 2:31 AM
Edtadros removed Edtadros as the assignee of this task.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Ventura
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: 'mw-ui-icon' prefix in favor of 'popups-icon'

screenshot 80.png (1×1 px, 672 KB)

✅ AC2: https://codesearch.wmcloud.org/search/?q=mw-ui-icon&files=&excludeFiles=&repos=Extension%3APopups should yield zero results

screenshot 81.png (559×1 px, 91 KB)

Edtadros updated the task description. (Show Details)Sep 22 2023, 2:33 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL