Page MenuHomePhabricator

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

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
1T213279#6094517
2T213279#6094517

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: Readers-Web-Backlog.
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

A similar issue on uploads.js

if ( $( '.errorbox' ).length === 0 ) {
Jdlrobson lowered the priority of this task from Medium to Low.Apr 9 2019, 4:29 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?

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

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

Jdlrobson added a subscriber: Jdlrobson.

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

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

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 :)

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 updated the task description. (Show Details)
Jdlrobson added a subscriber: Soda.
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 added a subscriber: Edtadros.

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.


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

BetaProd
You can see the text under "African wild dog" appears to run off the page, this does not happen on a physical device.
Edtadros updated the task description. (Show Details)Apr 29 2020, 5:38 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!