[EPIC] Refactor Hovercards (née Popups)
Closed, ResolvedPublic

Description

Plan (YMMV)

  1. Remove all module definitions from the codebase.
    1. The code for ext.popups.renderer.mobileRenderer and ext.popups.targets.mobileTarget (defined in PopupsHooks#onResourceLoaderRegisterModules) should be removed from the codebase.
  2. Add the ext.popups.lib module, which contains Redux@3.6.0.
  3. Define and document the set of events that Hovercards should respond to.
  4. Create actions for those events.
  5. Move all DOM event handlers into a single file and add unit tests.
    1. Each handler should dispatch an action against the store.
  6. Run QUnit tests in Node.js by repurposing I00045bb9: Add isolated unit testing with npm run test:unit.
  7. Create the controller module, which contains the code responsible for maintaining the entire state of the system (which link is being interacted with, whether the link's title attribute should be scrubbed, timers, the render cache, the popup element and if it's visible or is being animated, etc.) by consuming ext.popups.event events and triggering actions when certain state transitions occur, i.e.
var controller = createController( /* initialState = */ {} );
mw.trackSubscribe(
  'ext.popups.event',
  event => controller.onEvent( event )
);
  1. Extract the API-related code from ext.popups.renderer.article to the api module.
    1. api subscribes to updates from the controller and makes an API request if the state has the correct shape. An event is dispatched when the API request is made, completes, or encounters an error.
    2. Ensure that anchor title attribute manipulation and hook firing code is removed.
  2. Extract the rendering code from the ext.popups.renderer.article module to the renderer module.
    1. renderer subscribes to updates from the controller and renders a Hovercard if the state has the correct shape.
    2. mw.popups.render( data ) returns a DOM element ready to be inserted into the DOM.
    3. The results of mw.popups.render may be cached in order to improve rendering performance but the caching behaviour should be distinct from the rendering behaviour.

Considerations

  1. Hovercards is event driven. We should consider using RxJS to simplify a lot of the logic around subscribing to streams of DOM events and timers.
  2. We should strongly consider using Redux to implement controller.
    1. It has a well-defined language that we can use to talk about modelling applications.
    2. It's battle tested.
    3. It's 2kB.
    4. It's incredibly well documented. Indeed, I wish I could write documentation like that…

Etherpad

I'm keeping a diary/my notes in an Etherpad.

Acceptance criteria

Full requirements at: https://www.mediawiki.org/wiki/Beta_Features/Hovercards/Functionality (draft: https://docs.google.com/document/d/1A53OEJpjzjSgGKWTCwUPSqNopTNskr_i8oElwzOX9Uc/edit?ts=58123738#heading=h.p7d3kocgqxg8 )

Summary of the article:

  • Article summary will contain the first 525 characters from the lead paragraph of each article
  • After the first 525 characters, a horizontal gradient will be displayed to indicate continuation (as in related pages). More info here: https://phabricator.wikimedia.org/T67845
  • The first instance of bolded text within the article will appear bolded within a hovercard. Any other instance of bolded text will not appear bolded
  • More info here: https://phabricator.wikimedia.org/T141651
  • If an article has no lead paragraph, a generic hovercard will be used (more info below)

Visual Design Criteria

Page previews need visual design improvement in three areas

  1. depth of shadow
  2. no border
  3. transition time

New Properties

.mwe-popups {
box-shadow: 0 45px 70px -20px rgba(0,0,0,0.3), 0px 0px 1px rgba(0,0,0,0.5);
border:none;
}

Before:


After:

transition time
There are two time constants for Pagepreview to appear

  1. delay
  2. transition time after the delay

We need to reduce the second value from 0.3s to 0.2s and keep the delay constant.

Related Objects

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 2 2016, 12:35 PM
phuedx updated the task description. (Show Details)Nov 3 2016, 2:24 PM
phuedx updated the task description. (Show Details)Nov 3 2016, 4:57 PM
phuedx added a subscriber: jhobs.

After talking with @jhobs, we decided that making the dependency on Analytics-EventLogging hard would be ill advised.

phuedx updated the task description. (Show Details)Nov 3 2016, 4:59 PM
phuedx updated the task description. (Show Details)Nov 4 2016, 7:51 AM
phuedx updated the task description. (Show Details)Nov 4 2016, 10:18 AM
phuedx added a comment.EditedNov 4 2016, 10:47 AM

Rationale for #5

For me, the hardest part of debugging Hovercards issues (especially those concerning EventLogging) was understanding the system's state and how it's mutated given some interaction. This was in no small part because the state was defined, initialized, and mutated in various parts of the codebase.

Hovercards is meant to respond to user interaction (represented by DOMEvents by the UA), therefore this should be central to its design. Indeed, this is captured in #4, wherein we transform DOMEvents into a stream of events for one or more somethings to observe and respond to if necessary.

The state required for Hovercards to function isn't actually overly complicated and to keep things easy to debug/easy to reason about we'd like to distribute that it as little as possible throughout the codebase. Thus the ext.popups.controller is born: a something that observes the stream of events, updating the state accordingly and notifying other components that the state has been updated.

phuedx updated the task description. (Show Details)Nov 4 2016, 11:02 AM
phuedx updated the task description. (Show Details)
ovasileva triaged this task as High priority.Nov 7 2016, 2:26 PM
Tbayer added a subscriber: Tbayer.Nov 7 2016, 5:16 PM
phuedx added a comment.Nov 7 2016, 7:02 PM

I've created the mpga (Make Popups Great Again) branch.

phuedx updated the task description. (Show Details)Nov 7 2016, 7:07 PM
phuedx updated the task description. (Show Details)Nov 8 2016, 11:34 AM

What are the downsides of using RxJS and Redux?

What are the downsides of using RxJS and Redux?

External libraries, so they require security review. I don't think we're gonna need RxJS, but we're moving forward with Redux.

We should consider adding "works on Wikidata" as a requirement. Right now Wikidata uses a gadget to make Popups work, which recently broke after some of the earlier refactoring we did on master. It will, of course, break again with the larger refactor, so we may want to think of Wikidata support early on. I think all the the gadget does is add more classes to the link selector and then call a bunch of functions manually.

That being said, adding Wikidata support would be extending past feature parity, which is a trap we were worried about falling into, so maybe we should just plan it for the future?
/cc @phuedx @ovasileva

@jhobs, isn't Hovercards disabled when the gadget is on? The problem with Wikidata yesterday was that it was serving old JS files.

@jhobs, isn't Hovercards disabled when the gadget is on? The problem with Wikidata yesterday was that it was serving old JS files.

No, Hovercards are only disabled when the NavPopups gadget is enabled IIRC. The Wikidata gadget is called "PopupsFix." See T150401#2784883 from linked comment onwards for more info.

@jhobs - If we consider the "general page preview" (i.e. the one that says no preview is available), which should be built upfront - it would appear on almost everything on wikidata. The popupsfix seems like a good solution, but I'm okay turning them off for the moment and postponing a fix. Also, does anyone have an example of popupsfix in action - I tried to enable it but don't see any difference in the hovercards?

Elitre added a subscriber: Elitre.Nov 10 2016, 5:34 PM

adding Wikidata support would be extending past feature parity, which is a trap we were worried about falling into

Maintaining feature parity is a good thing. Will that include maintaining the requirement that the tool be structured so that it's possible later to make it work like the real Popups (which is https://en.wikipedia.org/wiki/WP:POPUP ), or is that going to be lost?

I really don't want to see us re-write Hovercards now, and then have to re-write it again in a couple of years when someone else says, "Oops, maybe the original team had that requirement for a good reason..."

Ping @Prtksxna – just so that you're aware that this is going on 🙂

ovasileva updated the task description. (Show Details)Nov 16 2016, 5:24 PM
This comment was removed by ovasileva.
ovasileva updated the task description. (Show Details)Nov 17 2016, 3:59 PM

@phuedx, @jhobs - updated acceptance criteria

ovasileva updated the task description. (Show Details)Nov 18 2016, 1:09 PM

Ping @Prtksxna – just so that you're aware that this is going on 🙂

Thanks! And, wow :)

Tbayer updated the task description. (Show Details)Nov 29 2016, 10:11 PM
Tbayer added a comment.EditedNov 29 2016, 10:14 PM

Now that the functionality document has been moved from Google Docs to https://www.mediawiki.org/wiki/Beta_Features/Hovercards/Functionality , I assume that this is the main location which will be updated from now on. I just did so with the results from today's meeting about EL: https://www.mediawiki.org/wiki/Beta_Features/Hovercards/Functionality#EventLogging (will update the schema page on Meta too)

phuedx renamed this task from Refactor Hovercards (née Popups) to [EPIC] Refactor Hovercards (née Popups).
kaldari added a subscriber: kaldari.Dec 6 2016, 3:50 PM

This is awesome and much needed. One bike-shedding question: Why the extreme drop-shadow change? OOjs-UI has moved to extremely small (almost non-existent) drop-shadows, while Hovercards is moving to extremely large drop-shadows. It seems like there should be some consistency. @Prtksxna.

Nirzar added a comment.Dec 6 2016, 6:02 PM

@kaldari

This is awesome and much needed. One bike-shedding question: Why the extreme drop-shadow change? OOjs-UI has moved to extremely small (almost non-existent) drop-shadows, while Hovercards is moving to extremely large drop-shadows. It seems like there should be some consistency.

context. page previews need to take focus away from base content. production page previews get mixed together with the article. we need depth. also we cannot have one single shadow for all use cases we have. page preview is a special case of a popover element where the popover is a content piece..

Nirzar added a comment.EditedDec 6 2016, 6:06 PM

@kaldari oh the mock in the description is old. we revised it and toned it down a bit. updated the mock now.

Nirzar updated the task description. (Show Details)Dec 6 2016, 6:08 PM

@Nirzar: What's the difference between floating elements like the notifications popup and hovercards? Shouldn't those be fairly similar? From the user's POV, they seem like virtually the same type of widget, but one has a tiny hard drop-shadow and the other has a large soft drop-shadow. I'm fine with whatever your decision is, but just wanted to make sure you're aware of the apparent inconsistency.

the notifications popup and hovercards

One has article content and the other has chrome / ui elements. For wikipedia, content is more important than chrome. thus, a special case of a popover. It's a feature in itself. One UI element is a feature, because of what it carries.

Switching focus between 2 pieces of content is what page previews do. I have written a long write up about this called "on context and consistency" for our UISTD efforts. layer hierarchy is something we have on our list to define guidelines for. what elements we consider on-brand is a long discussion.

I also used this shadow for a month for my personal use (as a heavy page previews user). I would be happy to share more details on context, brand, and consistency conversations we have, if you are interested.

aware of the apparent inconsistency.

it's actually not an inconsistency. shadows are used for hierarchy like font sizes are used for hierarchy. it's like saying, we have inconsistency between h1 font size and body font size :)

Prtksxna removed a subscriber: Prtksxna.Feb 28 2017, 3:35 PM
Jdlrobson added a subscriber: Jdlrobson.

@phuedx I think we've resolved this epic.
Want to do the honors of resolving?

@phuedx I think we've resolved this epic.
Want to do the honors of resolving?

phuedx closed this task as Resolved.Apr 13 2017, 3:30 PM