Some notes from Baha:
>>! In T133022#3269470, @bmansurov wrote:
>
> [[ https://phabricator.wikimedia.org/diffusion/EPOP/browse/master/src/renderer.js;49c4cb4d42bd989fb9bae822e9c2244248e2378c$261 | The show function ]], for example, is responsible for too many things. It is too complicated as it accepts seemingly unrelated parameters, creates a layout, layouts a preview, binds some behavior, etc.- Replace inlined SVG string
> - Add svg-inline-loader to Webpack config (referencing [[ https://gerrit.wikimedia.org/r/plugins/gitiles/marvin/+/master/src/client/webpack.config.ts#141 | Marvin ]])
> [[ https://phabricator.wikimedia.org/diffusion/EPOP/browse/master/src/renderer.js;49c4cb4d42bd989fb9bae822e9c2244248e2378c$294 | Binding a behavior as part of showing a preview ]] hasn't been tested. In general functions make use of other functions in the file without making them explicit dependencies, unlike data passed to functions. Passing dependent functions to other functions is also not practical as the function signature gets even more complicated. - Move the SVG in createPokeyMasks() to an SVG file
> - Add documentation explaining that this is a fancy mask to point at the link
> [[ https://phabricator.wikimedia.org/diffusion/EPOP/browse/master/src/renderer.js;49c4cb4d42bd989fb9bae822e9c2244248e2378c$370 | The createThumbnail ]] function has two many code paths. It takes enourmous effort in order to understand the function correctly and change something in it. Although tests try to cover all cases, we are only creating one case for each code path. [[ https://phabricator.wikimedia.org/diffusion/EPOP/browse/master/src/renderer.js;49c4cb4d42bd989fb9bae822e9c2244248e2378c$586 | getClasses ]] is another such function.
>
> What I'd like to see is we break up functions into small chunks that do one thing well and we test those funtions with random input. Testing monstrous functions with random input will be a major pain. I suggest we use a property-based testing library, perhaps [[ https://jsverify.github.io/ | JSVerify ]] - The pokey is displayed adjacent to the image (“touching”), for generating random test data.
I suggest creating subtasks for code that is going to be revised,the pokey contains part of the image (https://drive.google.com/file/d/1HH4LjqIdfOa3eTPLgXKPOK1rRyX8Wgac/view). to slowly chip away at this.
NOTE: Part of this work should be informed by {T186129} – it should at least be considered while refactoring the rendererSVG masks are used in place of CSS masks for browser support issues (see https://caniuse.com/#feat=css-masks).
== Related Technical Debt
* `layoutPreview` doesn't necessarily need to use jQuery to do `class` attribute manipulation when `Element.classList` is supported by all Grade A browsers.- Reword files to consistently refer to popups and previews
* `Function#bind`, which is now supported by all Grade A browsers, can be used to avoid `Renderer#show` and `Renderer#hide`.
== Acceptance criteria - Popups is a generic term that should be used whenever referring to common code
[] Behaviour - Previews is a specific term that should be separated from presentationused whenever referring to the current usage in desktop ("page previews")
[] Components of the hovercard should be separated e.g. a preview should make use of thumbnail component
[] Important: Whoever works on this should plan to setup a hangout with developers to share their thoughts and allow time for input prior to code review. - There may be fewer offenders than originally supposed but this work should at least include documenting the distinction
== Developer notes- Componentize UI
- Extract settings dialog, thumbnail, and popups into distinct components from their renderers
* Ideally createPreview and createEmptyPreview should be the only things exposed by ui/renderer.js - Separate render (appending to DOM) and binding events from creation
* Consider creating ui/behaviour.js to capture things such as showing the preview at a certain location. - Replace Mustache with ES6 template strings
** It's also plausible that this could be done via the reducer and passed into createPreview - Add ES6 transpilation step for string template usage; remove ES5 ESLint config
* Consider creating components for a preview - extract, thumbnail and footer - Convert Mustache templates to template strings
* Consider dropping use of mustache library as part of this change in favour of pure DOM or compiled templat - If this is difficult, use Preact as an exercise
- Document JavaScript filesizes on release timeline
- Note: it’s also about the MediaWiki payload size. Given Popups is the only thing in desktop production using mustache it may be cheaper (in terms of bytes) and more manageable to not use it.It’s depending on mediawiki.template.mustache
Notes on triggering for QA:
- https://drive.google.com/file/d/10skTokq_kOjIu9C8wg7DTHt73YjvScmS/view?usp=sharing