Page MenuHomePhabricator

One last tweak to survey placement..
Closed, ResolvedPublic

Description

Changes needed:

  1. http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_with_image_no_infobox

Currently: appears at bottom of lead section
Expected on desktop: survey appears floated to right above the image of the cat
Expected on mobile: survey appears above the image of the cat full width

  1. When the screen resolution is less thn 768px a survey will always be inserted after the first paragraph if available. If not, it will follow the same existing logic

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to To Do on the Reading-Web-Sprint-56-Four Lions board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 24 2015, 8:03 PM
Jdlrobson triaged this task as Normal priority.Sep 24 2015, 8:08 PM
rmoen set Security to None.
rmoen moved this task from To Do to Doing on the Reading-Web-Sprint-56-Four Lions board.

Change 240921 had a related patch set uploaded (by Robmoen):
Final tweaks to QuickSurvey panel placement.

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

I'll pick up the card since Rob is out today. I hope that's OK since we want to get this merged before today's deployment.

a few suggested improvements on the patch
also would be great if you could +2 https://gerrit.wikimedia.org/r/241900

Change 240921 merged by jenkins-bot:
Correctly place the QuickSurvey panel

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

Two issues came up in testing:

http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_stub?quicksurvey=true the survey shows at bottom.
Expected: after first paragraph.

http://en.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_without_lead_section?quicksurvey=true
Expected: in lead section consistent with mobile.

(Also if someone could merge https://gerrit.wikimedia.org/r/242370 that would help with testing)

bmansurov added a comment.EditedSep 30 2015, 10:43 AM

Two issues came up in testing:
http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_stub?quicksurvey=true the survey shows at bottom.
Expected: after first paragraph.

This is a caching issue. For example, http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_stub?quicksurvey=true&x works.

http://en.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_without_lead_section?quicksurvey=true
Expected: in lead section consistent with mobile.

This is also related to old HTML. The new html should not contain empty paragraphs in this article as the name suggests the article does not have a lead section. Re-running browser tests should fix this problem.

Two issues came up in testing:
http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_stub?quicksurvey=true the survey shows at bottom.
Expected: after first paragraph.

This is a caching issue. For example, http://en.m.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_stub?quicksurvey=true&x works.

Not true. I purged the HTML using action=purge so now I have new HTML and the problem is still there.
With &x I still see the same - the survey is at the bottom of the page rather than after the first paragraph..

http://en.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_without_lead_section?quicksurvey=true
Expected: in lead section consistent with mobile.

This is also related to old HTML. The new html should not contain empty paragraphs in this article as the name suggests the article does not have a lead section. Re-running browser tests should fix this problem.

Also not true.
http://en.wikipedia.beta.wmflabs.org/wiki/Quick_survey_test_page_without_lead_section?quicksurvey=true

Change 242782 had a related patch set uploaded (by Bmansurov):
Insert the panel after the lead section if it exists

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

@Jdlrobson, you were right. I could replicate both problems after a hard refresh. I've submitted a new patch, which takes care of those problems.

It looks like you've worked out the issues that are causing these 2 problems but I think you've overcomplicated your solution. I'd expect this fix to be no more than 5 lines added.

Change 242940 had a related patch set uploaded (by Jdlrobson):
Tweak positioning of Quick Survey

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

Whilst in this headspace @bmansurov I submitted an alternative patch which seems to fix the problems ^

bmansurov reassigned this task from bmansurov to Jdlrobson.Oct 4 2015, 7:24 AM

@Jdlrobson, I've left some comments on your patch. I'm assigning this to you since your patch looks cleaner.

Change 244583 had a related patch set uploaded (by Jdlrobson):
Fix placement of quick survey

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

I captured the bug @phuedx found in T115058 - I don't think it's a big blocker.
I rewrote the patch with QUnit tests (yay!) so I will be very surprised if you find any issues in https://gerrit.wikimedia.org/r/244583
Please be sure to review its parent commit which just adds tests for the status quo before fixing the edge cases

242940 is C:-1. @Jdlrobson: If you'd like to talk about/pair on this then that'd be great.

Change 242782 abandoned by Bmansurov:
Insert the panel after the lead section if it exists

Reason:
See https://gerrit.wikimedia.org/r/#/c/244583/

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

Change 244583 merged by jenkins-bot:
Fix placement of quick survey

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

Jdlrobson closed this task as Resolved.Oct 14 2015, 9:56 PM

Need to do a release before signing off. I'm waiting on T114485 before doing that.

Can someone merge https://gerrit.wikimedia.org/r/248986 so I can sign this off? @bmansurov @Jhernandez given that Sam is away that falls to one of you...