Page MenuHomePhabricator

Allow users who have navigational popups to use hovercards
Closed, ResolvedPublic3 Story Points

Description

Story

As a navpopups user, I want the ability to try out the hovercards feature, to see which one I like better

Solution

Navpops users won't be able to enable Page Previews unless they disable Navigation Popups gadgets from gadgets tab. We cannot disable it for users, the user has to do it manually. we can link to the gadgets tab though.

Here's how it will look


https://zpl.io/ZMFxTy

The blue link that says "disable navigation popups" will link to https://en.wikipedia.org/wiki/Special:Preferences#mw-prefsection-gadgets or just the hashtag "mw-prefsection-gadgets"

Acceptance Criteria

  1. If user had navigational popups enabled
    • Disable "previews on" option
    • Display message: "For page previews to work, please disable navigational popups from [the gadgets tab]"
    • Selecting link will navigate to gadgets
    • We log this as part of the Popups instrumentation
  2. If we DO NOT KNOW whether the user has navigational popups enabled:
    • Enable both options
    • Display message: "Certain gadgets and other customization may affect the performance of this feature. If you notice issues with page previews, please go through your gadgets list and other customization"

Plan

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Nirzar claimed this task.
Nirzar updated the task description. (Show Details)Nov 28 2016, 10:32 PM

@ovasileva design done. can you add Acceptance criteria?

@Nirzar - looks good! adding criteria

ovasileva updated the task description. (Show Details)Nov 29 2016, 5:33 PM
MBinder_WMF set the point value for this task to 3.Nov 29 2016, 5:53 PM
pmiazga claimed this task.
Nirzar added a comment.EditedDec 21 2016, 7:46 PM

@pmiazga

<td class="mw-input">

<div class="mw-htmlform-flatlist-item disabled">
  <input type="radio" disabled>&nbsp;
  <label >Enable</label>
</div>

<div class="mw-htmlform-flatlist-item disabled">
  <input  type="radio" disabled  checked="checked">&nbsp;
  <label >Disable</label>
</div>

</td>

=== CSS ===
div.mw-htmlform-flatlist-item.disabled {
opacity: 0.5;
}
note: the mw-htmlform-flatlist item is an existing class

for the tip under the enable disable items
<tr class="mw-htmlform-field-HTMLCheckField">
  <td class="mw-label"><label>&nbsp;</label></td>
    <td class="mw-input">&nbsp;
      <label for="mw-input-wpmultimediaviewer-enable">You need to <span class="plainlinks"><a class="external text" href="#mw-prefsection-gadgets">Disable Navigation popup gadget</a> from Gadgets tab to enable page previews</span>
      </label>
  </td>
</tr>

Change 330432 had a related patch set uploaded (by Pmiazga):
Disable Page Previews preferences when NavPopups are enabled

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

Change 331371 had a related patch set uploaded (by Pmiazga):
Update enable previews copy

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

ovasileva updated the task description. (Show Details)Jan 9 2017, 7:17 PM
ovasileva added a subscriber: phuedx.

@phuedx, @pmiazga - acceptance criteria updated

Change 331371 merged by jenkins-bot:
Update enable previews copy

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

Am a little lost with what needs merging to call this complete but https://gerrit.wikimedia.org/r/#/c/328675/4 is holding up the merging of a few other patches. Am waiting for a reply from @phuedx.

@Jdlrobson: rEPOP211f1d16589b: Disable Page Previews preferences when NavPopups are enabled needs testing and merging. I'll be doing the former in the next half hour.

Change 330432 merged by jenkins-bot:
Disable Page Previews preferences when NavPopups are enabled

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

phuedx added a comment.EditedJan 11 2017, 4:16 PM

@pmiazga: IIRC we discussed making Popups\PopupsGadgetsIntegration::NAVIGATION_POPUPS_NAME configurable as the name of the NavPopups gadget varies by wiki.

@phuedx: yes and it was done, looks like I lost couple changes after rebasing https://gerrit.wikimedia.org/r/330432, let me check that

@ovasileva: Also, if $wgPopupsBetaFeature is true, then should the beta feature be disabled? If so, then what does a disabled beta feature look like? /cc @Nirzar

Change 331695 had a related patch set uploaded (by Pmiazga):
Move conflicting NavPopups gadget name into config var

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

Change 331695 merged by jenkins-bot:
Move conflicting NavPopups gadget name into config var

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

@phuedx I moved this task back to Needs More Work as it doesn't meet acceptance criteria yet.

@ovasileva @phuedx - could you elaborate on If we DO NOT KNOW whether the user has navigational popups enabled?
What to you mean by "do not know"? I tried to come with checks:

  • check if gadgets extension exists
  • check if user has defined global JavaScripts
  • check if NamespacePopups extension exists (looks like another popups extension)

Additionally in Github we host two extensions:

There are lots of different scenarios, instead of doing some overcomplicated checks [addinig necessary code complexity] I decided that we can show "Certain gadgets and other customization may affect the performance of this feature. If you notice issues with page previews, please go through your gadgets list and other customization" copy all the time.
We will show "For page previews to work, please disable navigational popups from [the gadgets tab]" copy only if user has *Navigational Popups* gadget enabled.

Navigational Popups gadget can be disabled but we are still not 100% sure that Navigational Popups is disabled as it might be enabled under a different name, or just can be added via global user scripts.

Change 331997 had a related patch set uploaded (by Pmiazga):
Inform users about conflicting gadgets/customizations

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

Change 331997 merged by jenkins-bot:
Inform users about conflicting gadgets/customizations

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

ovasileva closed this task as Resolved.Jan 18 2017, 1:34 AM

For the record, this is available to test here: http://reading-web-staging.wmflabs.org.

The tip text is appearing on left and a unreadable font size, can we match the alignment and fontsize with zeplin spec?

phuedx reopened this task as Open.Jan 18 2017, 5:22 PM

Per T151058#2948518 (above) and the following observation: the PopupsContext#conflictsWithNavPopupsGadget variable isn't forwarded to the client so that it can be included in the instrumentation. I think that this is relevant to this task – the method was introduced while working on this task – but is also tracked in T152225: Finish rewriting instrumentation, which was actually blocked by this task. Therefore, I'll reopen this task, update the AC, and resolve T152225: Finish rewriting instrumentation.

phuedx updated the task description. (Show Details)Jan 18 2017, 5:25 PM
phuedx updated the task description. (Show Details)

@pmiazga: I have a WIP for forwarding PopupsContext#conflictsWithNavPopupsGadget to the client. Do you want to take this? If so, then I'll discard my change.

Change 333792 had a related patch set uploaded (by Pmiazga):
Pass conflictsWithNavPopupsGadget to js client

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

Change 333792 merged by jenkins-bot:
Pass conflictsWithNavPopupsGadget to js client

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

Change 334131 had a related patch set uploaded (by Phuedx):
Add hovercardsSuppressedByGadget to logging

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

rEPOP6cc625270487: Add hovercardsSuppressedByGadget to logging (finally) gets rid of the "Missing property "hovercardsSuppressedByGadget"..." client-side exception.

Change 334131 merged by jenkins-bot:
Add hovercardsSuppressedByGadget to logging

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

I've updated the staging server. @ovasileva: Did you take a look at this last week?

phuedx reassigned this task from pmiazga to ovasileva.Jan 26 2017, 1:57 PM
phuedx added a subscriber: pmiazga.
ovasileva closed this task as Resolved.Jan 26 2017, 3:11 PM

just re-tested - all done!