Page MenuHomePhabricator
Create Task
Maniphest T213279

[Dev] mobile.special.watchlist.scripts/watchlist.js contains overly generic selectors
Closed, ResolvedPublic
Actions

Assigned To
ovasileva
Authored By
Esanders
Jan 9 2019, 1:07 PM
Tags
Referenced Files
F31787046: en.m.wikipedia.org_wiki_Special_EditWatchlist(iPhone X).png
Apr 29 2020, 5:37 PM
F31787042: en.m.wikipedia.beta.wmflabs.org_wiki_Special_EditWatchlist(iPhone X).png
Apr 29 2020, 5:37 PM
F31787044: en.m.wikipedia.beta.wmflabs.org_wiki_Special_EditWatchlist(iPhone X) (1).png
Apr 29 2020, 5:37 PM
Subscribers
Aklapper
Edtadros
Esanders
Jdlrobson
Jdrewniak
Soda
Volker_E

Description

$( '.more' ).remove();

could easily conflict with some other extension on the page

  1. Class names should be prefixed (mw-mf-)
  2. Global selectors should be avoided in JS. If the DOM node can't be kept in memory, selectors should at least be scoped to the application's root DOM node (e.g. $watchlist.find( '.mw-mf-more' ))

QA steps

QA Results - Beta|Prod

ACStatusDetails
1❓T213279#6094517
2✅T213279#6094517

Details

SubjectRepoBranchLines +/-
Replaced overly generic selectors, scoped selectorsmediawiki/extensions/MobileFrontendmaster+12 -10
Customize query in gerrit

Related Objects

Event Timeline

Esanders created this task.Jan 9 2019, 1:07 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 9 2019, 1:07 PM
Jdlrobson triaged this task as Medium priority.Jan 9 2019, 4:32 PM
Jdlrobson added a project: Web-Team-Backlog.
Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson added projects: good first task, patch-welcome.
Esanders added a comment.Jan 10 2019, 2:54 PM

A similar issue on uploads.js

if ( $( '.errorbox' ).length === 0 ) {
kostajh mentioned this in T219222: Make notification counts update without page reload.Apr 2 2019, 8:27 PM
Jdlrobson lowered the priority of this task from Medium to Low.Apr 9 2019, 4:29 PM
Jdlrobson edited projects, added MobileFrontend (MobileFrontend Special Pages); removed MobileFrontend.Oct 8 2019, 3:14 PM
Jdlrobson moved this task from Backlog to Watchlist on the MobileFrontend (MobileFrontend Special Pages) board.Oct 8 2019, 9:53 PM
Soda claimed this task.Apr 1 2020, 4:30 PM
Soda added a comment.Apr 2 2020, 4:06 PM

Do I keep the old classes also for backwards compatibility?

gerritbot added a comment.Apr 9 2020, 5:43 PM

Change 587823 had a related patch set uploaded (by Sohom Datta; owner: Sohom Datta):
[mediawiki/extensions/MobileFrontend@master] Replaced overly generic selectors, scoped selectors

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

gerritbot added a project: Patch-For-Review.Apr 9 2020, 5:43 PM
Volker_E subscribed.Apr 10 2020, 6:07 AM

@Soda Yeah, the classes are needed for applying CSS. That could be changed in a follow-up.

Jdlrobson edited projects, added Web-Team-Backlog (Kanbanana-2019-20-Q4); removed Web-Team-Backlog.Apr 15 2020, 3:09 PM
Jdlrobson subscribed.

Incoming code review request from outside the team. Looks like a good contribution!

Jdlrobson renamed this task from mobile.special.watchlist.scripts/watchlist.js contains overly generic selectors to [Dev] mobile.special.watchlist.scripts/watchlist.js contains overly generic selectors.Apr 15 2020, 3:09 PM
Jdlrobson moved this task from Needs Analysis to Needs Code Review on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.
Esanders added a comment.Apr 17 2020, 12:04 AM
In T213279#6022966, @Soda wrote:

Do I keep the old classes also for backwards compatibility?

Unless the classes are being used outside of the extension (e.g. in gadgets / user scripts / common.css/js ) then we don't need to keep the old classes. It seems unlikely that these classes are being used anywhere else, so you can probably just rename and remove the old names.

Soda added a comment.Apr 17 2020, 5:14 PM
In T213279#6064593, @Esanders wrote:
In T213279#6022966, @Soda wrote:

Do I keep the old classes also for backwards compatibility?

Unless the classes are being used outside of the extension (e.g. in gadgets / user scripts / common.css/js ) then we don't need to keep the old classes. It seems unlikely that these classes are being used anywhere else, so you can probably just rename and remove the old names.

Sure, I kept the classes in this time, I'll take the classes out in the next patches :)

gerritbot added a comment.Apr 17 2020, 5:34 PM

Change 587823 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Replaced overly generic selectors, scoped selectors

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

Jdlrobson reassigned this task from Soda to Edtadros.Apr 17 2020, 5:38 PM
Jdlrobson moved this task from Needs Code Review to Needs QA on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Soda.
ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.30; 2020-04-28).Apr 17 2020, 6:00 PM
Maintenance_bot removed a project: Patch-For-Review.Apr 17 2020, 6:11 PM
Soda added a comment.Apr 26 2020, 6:29 PM

Thanks a lot for the reviews! Will surely act on the feedback once this part has gone through :)

Edtadros reassigned this task from Edtadros to ovasileva.Apr 29 2020, 5:37 PM
Edtadros moved this task from Needs QA to Ready for Signoff on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.
Edtadros subscribed.

Test Result - Beta

Status: ✅ PASS pending clarification on AC1
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP, iPhone 11 Max
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

Disable AMC for this one.
Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:Watchlist?hidecategorization=1&hideWikibase=1&limit=250&days=3&urlversion=2 and get 50+ articles on your watch list (https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:EditWatchlist/raw will help)
❓ AC1: Check that the more button at bottom of page is styled correctly
I have included a screenshot below of the "more" button. I would need more information on what the correct style should be to determine if it is styled correctly.

en.m.wikipedia.beta.wmflabs.org_wiki_Special_EditWatchlist(iPhone X).png (2×1 px, 284 KB)

✅ AC2: Check the watchlist styling is consistent with production flagging any inconsistencies.

BetaProd
en.m.wikipedia.beta.wmflabs.org_wiki_Special_EditWatchlist(iPhone X) (1).png (2×1 px, 636 KB)
You can see the text under "African wild dog" appears to run off the page, this does not happen on a physical device.
en.m.wikipedia.org_wiki_Special_EditWatchlist(iPhone X).png (2×1 px, 723 KB)
Edtadros updated the task description. (Show Details)Apr 29 2020, 5:38 PM
Jdlrobson added a subscriber: Jdrewniak.Apr 29 2020, 6:12 PM

more button LGTM @Edtadros

You can see the text under "African wild dog" appears to run off the page, this does not happen on a physical device.

Looks like a new bug introduced in https://gerrit.wikimedia.org/r/592741 @Volker_E, @Jdrewniak and I will need to coordinate a fix. Not sure if we want to move this into needs more work until this happens, even though it's not quite related.

Jdlrobson closed this task as Resolved.Apr 30 2020, 5:06 PM

Thank you @Soda!

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 · Credits