Display section issues modal
Closed, ResolvedPublic3 Story Points

Description

Update [16/08]
Patch merged.

Update [08/08]
Originally in this tasks we had not considered subsections. Subsection issues required writing code to identify which section contents belongs to - much trickier than a 3 pointer. Some nitpicks on patches remain.


Background

In T191303: Mobile page issues - visual styling changes we noted that there are some issues with displaying section issues. This task is to clarify and apply the expected behavior

Acceptance criteria

  • Continue displaying section issues on the page
  • When a section issue is selected, only display issues from the given section in the page issues modal
  • Perform full QA for section issues
  • for the existing treatment, we should ensure we do not show section issues and do not show them inside the issues overlay when open (i.e. keep existing behaviour)

Testing steps

Group B
Ensure you have been bucketed in test group B (banner visible at top of Equity_release)

Group A

    • On Albert Sidney Johnston and Alberta "Page issues" gray text below page title should NOT show (current issues only displays for lead section). This should match what is currently in production.
  • On Equity_release ensure "This page has issues" banner shows at top and only contains one issue relating to citations. This should match what is currently in production.
  • On Pharmacovigilance ensure "This page has issues" banner shows at top and only contains three issues. This should match what is currently in production.
  • Check "Category:Use_American_English_from_January_2014" and "Talk:Pharmacovigilance" show a link under the title. This should match what is currently in production.

developer notes

Right now we only extract issues from the first section. We will need to run this code on every section when the user is in the new treatment.

Right now the issues overlay is tied to the route #/issues. We will need to expand the route to #/issues/《sectionNum》to link overlays to their corresponding section. Should be relatively straightforward but may require storing an object which links section id to list of issues (right now we store section 0s issues in a local one dimensional array).

Note on desktop there is currently no concept of sections. Here it's probably best to show all issues inside a single overlay linked to from all issues in page. Dont worry too much about desktop Minerva. Long term the parser will have sections and this problem will go away.

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson added a comment.EditedJul 11 2018, 8:54 PM
Would it be possible to have the title of the modal be "Section issues" for section issues? My apologies for not thinking of that earlier.

Note certain templates in the lead section do not apply to the entire page e.g. https://en.m.wikipedia.org/wiki/Template:Lead_rewrite
This is technically a "Section issue", no?
e.g. http://reading-web-staging.wmflabs.org/wiki/Politics_of_Ethiopia#/issues/0
How to handle these? Would "Page issues" be confusing here?

@Jdlrobson good call out. The way I'm thinking about it is more like "this issue has been placed at the Page/Section level". So it's less about the contents of the issue/template, and more just communicating where in the page it is. Does that make sense? I'm hoping this would alleviate any confusion of someone opening a section issue and not seeing the page issues listed.

alexhollender updated the task description. (Show Details)
alexhollender updated the task description. (Show Details)

Change 445301 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Section issues overlay has different heading

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

Jdlrobson removed alexhollender as the assignee of this task.

Change 445301 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Section issues overlay has different heading

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

Jdlrobson assigned this task to alexhollender.

On staging for design review!

Jdlrobson removed alexhollender as the assignee of this task.
Jdlrobson added a subscriber: Niedzielski.

@Niedzielski has pointed out that the implementation is wrong. http://readers-web-master.wmflabs.org/w/index.php?title=Pharmacovigilance&mobileaction=toggle_view_mobile#/issues/5 shows all issues in subsections as well
Also the url suggests the issues are in section 5, but actually they are in section 27 (note the url when you click the edit icon)

.

@Alex @Stephen i looked into the section issue. Looks like it's going to be very tricky to show subsection issues separately.
https://gerrit.wikimedia.org/r/445667 adds some consistency between editor sections and issues sections but doesn't resolve the subsection issues. I'm not sure whether that's worth pursuing.

I'm off on vacation now. Feel free to grab this while I'm gone.

Looks like it's going to be very tricky to show subsection issues separately.

I think it's fine to show subsection issues with section issues in a single modal.

Change 445667 had a related patch set uploaded (by Niedzielski; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Match section issues to section number

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

pmiazga claimed this task.Jul 16 2018, 5:13 PM

@Niedzielski has pointed out that the implementation is wrong. http://readers-web-master.wmflabs.org/w/index.php?title=Pharmacovigilance&mobileaction=toggle_view_mobile#/issues/5 shows all issues in subsections as well
Also the url suggests the issues are in section 5, but actually they are in section 27 (note the url when you click the edit icon)

.

@alexhollender, @Jdlrobson - This feels a bit odd to me. If we have an article like the one above - the issues here seem a bit repetitive, especially if there's no additional information on the particular issue.

pmiazga removed pmiazga as the assignee of this task.
pmiazga removed a project: Patch-For-Review.
pmiazga added a subscriber: pmiazga.

@Jdrewniak, @Niedzielski, feel free to fix this issue since you're already deep into page issues and @Jdlrobson is on holiday.

Niedzielski claimed this task.
Niedzielski removed Niedzielski as the assignee of this task.

@ovasileva agreed that the modal with section + subsection issues looks odd. I'm not sure how to handle these. I think it makes sense to distinguish between issues on the entire page from issues within sections, but if we start distinguishing sections from sub-sections it may get a bit tricky because I assume since we go all the way down to H4 there could be sub-sub-sub-section issues. Not sure where to draw the line?

Change 445667 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Match section issues to section number

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

We need to work out what to do about subsection issues https://phabricator.wikimedia.org/T197932#4443017
Two options (but not limited to) include [1] Making them work (tricky but possible with a new task); [2] De-duplicating issues of the same type (I think this is feasible) so they only show once

We need to work out what to do about subsection issues https://phabricator.wikimedia.org/T197932#4443017
Two options (but not limited to) include [1] Making them work (tricky but possible with a new task); [2] De-duplicating issues of the same type (I think this is feasible) so they only show once

I think option 2 makes the most sense. @Jdlrobson - do we distinguish between section and subsection issues? Another option would be to hide subsection issues altogether.

Should I update this task and re-estimate or should we create a new task? The scope has changed considerably and it might be worth talking about this in next grooming!

@alexhollender, @Jdlrobson - discussed this during kickoff. We think the best way forward would be to remove subsection issues. @alexhollender - we discussed this in person last week, adding a note here to confirm that - let me know if anything's changed since then.

We think the best way forward would be to remove subsection issues.

@Jdlrobson does this relate to your change for identifying issues by section level via 'h1,h2,h3,h4,h5,h6'? Maybe we can still leave the subsection issues inline but identify top level issues in the dialog by passing it h1 issues only?

Change 448012 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Allow subsection issues

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

After much wrestling with the code, I think I've got section AND subsection issues working. It turns out hiding subsection issues isn't really feasible because of the way the page is structured. I'm not a big fan of the code I've produced here, but to be honest, I feel that way about all of the page issues code. I need to go lie down now ;-)

After much wrestling with the code, I think I've got section AND subsection issues working. It turns out hiding subsection issues isn't really feasible because of the way the page is structured. I'm not a big fan of the code I've produced here, but to be honest, I feel that way about all of the page issues code. I need to go lie down now ;-)

@Jdlrobson - what is the treatment for subsection issues that you tried? Do they appear at each subsection?

@ovasileva here is a screencast of how it behaves:

@ovasileva here is a screencast of how it behaves:

got it - that's perfect! @alexhollender - any thoughts from your side?

Looks great 👍

Jdlrobson claimed this task.Aug 1 2018, 8:50 AM

This has turned into an 8 pointer :)

Change 450037 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Allow querying of Page sections

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

Jdlrobson updated the task description. (Show Details)Aug 8 2018, 5:21 PM
Jdlrobson updated the task description. (Show Details)Aug 8 2018, 5:34 PM

Change 450037 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Allow querying of Page sections

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

nray added a subscriber: nray.Aug 13 2018, 5:23 PM

I reviewed this and added a +1 but didn't feel comfortable with +2 as its with code and AB testing that I'm pretty unfamiliar with so hopefully someone with more knowledge of this can add their 2 cents

Change 448012 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Allow subsection issues

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

Jdlrobson reassigned this task from Jdlrobson to alexhollender.EditedAug 16 2018, 7:39 PM
Jdlrobson removed a project: Patch-For-Review.

over to you @alexhollender for design review. Please test on http://reading-web-staging.wmflabs.org/)

Jdlrobson updated the task description. (Show Details)Aug 16 2018, 7:39 PM

Group B
Ensure you have been bucketed in test group B (banner visible at top of Equity_release)

  • Visit page https://reading-web-staging.wmflabs.org/wiki/Equity_release. This page has both a page issue and a section issue (in the "United States" section). Clicking the page issue at the top of the page should lead to a modal with only a description of that issue. Likewise for the section issue. Neither of the modals should contain descriptions of both issues.

Page issue:


Section issue:

  • Visit https://reading-web-staging.wmflabs.org/wiki/Pharmacovigilance. This page has multiple page issues (3) as well as section issues (the first five sections each have an issue). Again click the issues and check the modal to make sure that only the issue descriptions pertaining to the respective issues are appearing. So each of the section issues should only display one issue in their modals.

Multiple page issues:


Section issues:

About this page shows on both of these articles

Group A

  • On Albert Sidney Johnston and Alberta "Page issues" gray text below page title should NOT show (current issues only displays for lead section). This should match what is currently in production.

Albert Sidney Johnston - text does not show below title or below section title

Alberta - text does not show below title or below section title

  • On Equity_release ensure "This page has issues" banner shows at top and only contains one issue relating to citations. This should match what is currently in production.

Only one issue is displayed in modal

  • On Pharmacovigilance ensure "This page has issues" banner shows at top and only contains three issues. This should match what is currently in production.

Modal contains only three issues

  • Check "Category:Use_American_English_from_January_2014" and "Talk:Pharmacovigilance" show a link under the title. This should match what is currently in production.

About this page appears and is functional on both articles

It works! Moving to signoff

phuedx removed Ryasmeen as the assignee of this task.Aug 22 2018, 2:40 PM
phuedx added a subscriber: Ryasmeen.

@ovasileva: Since you're the PO and you've just QA'd this, who do you think should sign it off?

hmm, maybe @alexhollender, or really anyone that hasn't worked on the task?

alexhollender closed this task as Resolved.Aug 22 2018, 3:42 PM
alexhollender claimed this task.

Signing this off

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptAug 22 2018, 3:42 PM

@alexhollender - just to be as thorough as possible, checking off the acceptance criteria

ovasileva updated the task description. (Show Details)Aug 22 2018, 3:46 PM