Page MenuHomePhabricator

Design updates to Settings page & AMC opt-in toggle
Closed, ResolvedPublic2 Story Points

Description

Description

A few small tweaks to the Settings page. Note: these changes apply to ALL users (regardless of whether or not AMC mode is on/off).

Design

Design notes

Developer notes

  • This task will empty $wgMFSpecialPageTaglines however after talking to @alexhollender the code should be kept as it will be used for other special pages.
  • Mostly HTML, CSS tweaks that should be limited to resources/mobile.special.mobileoptions.styles/mobileoptions.less.

QA instructions

Environment: reading-web-staging
Browser & device: mobile (iOS and Android), all browsers // desktop, Chrome & Safari
Skin: MFE/Minerva
Other: please review as:

  • logged in & logged out
  • AMC on & AMC off

Steps:

  1. Access the mobile settings page by clicking the main navigation button and then "settings"
  2. Ensure the length of the text beside each input roughly aligns with the mockup
  3. Ensure the heading spacing aligns with mockup

Sign off step

  • Ensure next steps for $wgMFSpecialPageTaglines have been documented in a task.

QA Results

StatusDetails
✅ PassedT214195#4983642

QA Results:Production(idwiki)

StatusDetails
✅ PassedT214195#5064971

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptJan 18 2019, 6:38 PM
ovasileva removed pmiazga as the assignee of this task.Jan 18 2019, 6:55 PM
ovasileva removed the point value for this task.
ovasileva moved this task from Incoming to Upcoming on the Readers-Web-Backlog board.
Jdlrobson moved this task from Upcoming to Needs Prioritization on the Readers-Web-Backlog board.

Todo

Removing the tagline at the top of the page raises the question about whether the code that supports it ($wgMFSpecialPageTaglines) is needed any more as Special:MobileOptions is the only consumer.

@Jdlrobson and @alexhollender will go over T169162 and related tasks before estimation to work out this.

Todo

Removing the tagline at the top of the page raises the question about whether the code that supports it ($wgMFSpecialPageTaglines) is needed any more as Special:MobileOptions is the only consumer.
@Jdlrobson and @alexhollender will go over T169162 and related tasks before estimation to work out this.

@alexhollender - do we want to remove it for everyone (logged-out users included)? I don't really see it adding much value currently. Was it there to distinguish between preferences on desktop?

Jdlrobson removed Jdlrobson as the assignee of this task.Jan 22 2019, 10:08 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

@ovasileva thanks for the clarification — yes, we should remove it for all users.

ovasileva updated the task description. (Show Details)Jan 23 2019, 5:00 PM
ovasileva updated the task description. (Show Details)Jan 23 2019, 6:10 PM
alexhollender added a comment.EditedJan 23 2019, 6:12 PM

clarifying further, all changes described here should be made for everyone (AMC and non-AMC).

Change 486402 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Adjust heading padding for special pages

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

Change 486403 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Remove tagline from Special:MobileOptions page

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

Jdrewniak added a comment.EditedJan 25 2019, 12:29 AM

@alexhollender part one of this two part two-pointer, the headings.

I felt there was already a lot of CSS rules touching this header:


and I really didn't want to create another special case for this special page, so I ignored the guidance in the description a bit:

"CSS tweaks that should be limited to resources/mobile.special.mobileoptions.styles/mobileoptions.less."

And instead made the changes apply to the heading on all special pages. Now that might sound like scope creep, but these are small changes (just removing some padding from the top, and adding some to the bottom) and this approach allowed me to remove more code than I added. Hurra.

I don't have this up on a staging server yet, but the gist of it looks like this (change to heading padding only):

pagecurrent vs. new
Special:MobileSetting
Special:History
Special:MobileDiff
Special:Contributions
Special:RecentChanges
Special:Nearby
(not enabled on my local, but just image some padding below the heading)

There's obviously still a fair bit of irregularities across the special pages, but my hope this that this bit of extra bottom padding is desirable and a step towards some semblance of consistency, (and where there's too much we might be able to fix by using a tagline instead, etc.)

They both look good to me so marked WIP and +2ed. Feel free to remove WIP when Alex has signed off!

Should this be moved to *Needs Design Review* per ☝️?

@phuedx yes! meant to do that..
alrighty, and for part two of this two pointer, the text width.

I noticed T180652 being referenced in the comments and I realized OOUI doesn't afford us much flexibility when it comes to the width of the columns, (it's a 40/60 split, take it or leave it) so I decided to override that by using a flex-box layout to position the labels & inputs beside each other. While I was at it I also vertically centred the icons because they weren't very well aligned to the top anyway.

before / afterWith AMC toggletablet sizealignment! huzza! (new on bottom)

@alexhollender do these screenshots suffice for a design review? If not I can try to put these patches up on a server somewhere.

Change 486474 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Modify Special:MobileOptions layout

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

@Jdrewniak thanks for the comprehensive comments and screenshots. The updates you've made look great. A few things:

  1. It seems that we've lost the spacing below the page title. Maybe related to T214955.
iOS SafariAndroid
  1. I'm having a difficult time determining if the expanded column width is working on iOS/Safari. If you compare the two screenshots above it doesn't seem to be. On MacOS/Safari it seems to work. Bit confused there.
  1. Can you update the name of the setting to "Advanced contributions mode" (currently it is "Advanced Mobile Contributions")?
  1. For sanity can you include a screenshot of what a special page with a tagline would look like given the changes you're making here?

Just leaving a note to double-check the settings page altogether whenever this gets to QA

@alexhollender I've updated readers-web-staging with the following patches, there was an error checking the Minvera patch out the last time. The spacing below the title should be ok now.

Currently on staging:

  1. MobileFrontend patch - removes the tagline - https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/486403/
  2. MobileFrontend patch - fixes spacing between inputs and labels - https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/486403/
  3. Minerva Patch - adjusts the heading-padding and settings layout - https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/486402/

https://reading-web-staging.wmflabs.org/w/index.php?title=Special:MobileOptions&returnto=Special%3AVersion&mobileaction=toggle_view_mobile

FYI you have to log in to see the AMC toggle. I can update AMC label in a followup patch (don't want to mess with the existing ones right now).

To see a special page with a tagline, I think it would be easiest to edit that specific HTML from the devtools. We still render and empty div below the heading for the tagline, so just clicking "edit as HTML" on that element should give you an idea of what it looks like.

Just spoke with @Jdrewniak on Slack. Two remaining items:

  • Decrease padding on the right of the oo-ui-fieldLayout-header element
  • Update title of setting to "Advanced contributions mode" (this will be done in a followup patch)

Change 486474 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Modify Special:MobileOptions layout

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

Change 486403 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Remove tagline from Special:MobileOptions page

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

Change 486402 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Adjust heading padding for special pages

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

Jdrewniak removed Jdrewniak as the assignee of this task.Feb 11 2019, 1:35 PM
Jdrewniak updated the task description. (Show Details)

@Jdrewniak well, somehow this thing keeps changing. Now there is extra spacing below the title (and title+tagline element). 15px in both cases.

without tagline - the tagline element is still holding space, if you remove this it's all setwith tagline - if you remove the padding on the bottom of the tagline it's all set

Change 491451 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Remove special .tagline styles on mobile user page

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

Awesome, looks like we've finally got it.

settingssettings+taglinewatchlistcontributions
iOS
Android

Change 491451 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove special .tagline styles on mobile user page

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

Jdlrobson reassigned this task from Jdrewniak to Edtadros.Feb 20 2019, 12:44 AM

This has no implications on cached HTML as the new styles only impact AMC. I'm not seeing any visible problems with or without taglines in stable.
Over to Edward for some testing. I've updated on staging.

@Jdlrobson these changes should apply to all users (logged in and logged out, AMC on/off). I've updated the task description and QA instructions. Let me know if you have any concerns.

Edtadros reassigned this task from Edtadros to alexhollender.EditedFeb 26 2019, 6:29 AM
Edtadros added a subscriber: Edtadros.

Test Result

NOTE: AC2 initially was set to Fail, the AC was clarified by @Jdrewniak and @alexhollender in T214195#4984108 T214195#4984556 respectively.

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome DevTools Device Emulator (iPhone X)

Test Artifact(s):

Logged In with AMC off:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockupT214195#4984108 T214195#4984556
3Ensure the heading spacing aligns with mockup

Logged In with AMC on:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockupT214195#4984108 T214195#4984556
3Ensure the heading spacing aligns with mockup

Logged off:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockupT214195#4984108 T214195#4984556
3Ensure the heading spacing aligns with mockup

thanks @Edtadros !

@alexhollender can I get a confirmation that it's OK that:

  1. the width of the text description is variable, depending on the width of the widget beside it?
  2. The padding between the text and the widget is constant at 1.5em

I believe we discussed this offline, but it's better to have it on the record, since it's slightly different than the mock :)

@Jdrewniak thanks for the explanatory screenshot here. Yes, that strategy sounds good to me.

Edtadros reassigned this task from Edtadros to ovasileva.Feb 26 2019, 2:58 PM
Edtadros updated the task description. (Show Details)

@Jdrewniak and @alexhollender thanks for clarifying AC2. I set this to Pass.

ovasileva closed this task as Resolved.Feb 26 2019, 4:26 PM

In that case, looks like we can resolve

Test Result:Production

Status: ✅ PASS
OS: iOS
Browser: Chrome browser on (iPhone XS Max)

Test Artifact(s):

Logged In with AMC off:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockup
3Ensure the heading spacing aligns with mockup

Logged In with AMC on:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockup
3Ensure the heading spacing aligns with mockup

Logged off:

StepDescriptionStatusArtifact
1Access the mobile settings page by clicking the main navigation button and then "settings"
2Ensure the length of the text beside each input roughly aligns with the mockup
3Ensure the heading spacing aligns with mockup
Edtadros updated the task description. (Show Details)Mar 28 2019, 9:08 AM