Page MenuHomePhabricator

Simplify CategoryOverlay code for readability
Closed, ResolvedPublic3 Estimated Story Points

Description

Web team spent some time looking in detail at the category overlay.

We identified various ways of improving this code as a springboard to improving the rest of the code.

This task begins that work:

The work here is is focused to:

  • removing unnecessary inheritance
  • renaming variables
  • refactoring existing logic
  • improving readability
  • removing templates where necessary
  • removing the unnecessary M.on call for the category save event

This task is timeboxed. We should look to spend no more than 3 days in the doing column.

Notes from discussion

Event Timeline

Jdlrobson renamed this task from Simplify CategoryOverlay to Simplify CategoryOverlay for readability.Apr 11 2018, 3:13 PM
Jdlrobson triaged this task as Medium priority.
Jdlrobson created this task.
Jdlrobson moved this task from Incoming to Upcoming on the Web-Team-Backlog board.
Jdlrobson set the point value for this task to 3.Apr 24 2018, 4:22 PM

This task is timeboxed. We should look to spend no more than 3 days in the doing column.

We checked in on this today and it sounds like due to other distractions, although this has been on the board for 3 days it's not actively been worked on, so we're good, right now.

Change 460547 had a related patch set uploaded (by Niedzielski; owner: Vagrant Default User):
[mediawiki/extensions/MobileFrontend@master] Hygiene: limit CategoryOverlay's inheritance

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

Change 460548 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: limit CategoryOverlay's inheritance

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

Change 460551 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename View.options.enhance to skipTemplateRender

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

Change 460568 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove unused Overlay.defaults.fixedHeader

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

Change 460574 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: document CategoryOverlay code w/ function

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

Change 460575 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename Overlay.clearSpinner() to hide

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

Change 460986 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: a few CategoryOverlay bugs and refactor

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

Change 460551 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename View.options.enhance to skipTemplateRender

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

Jdrewniak renamed this task from Simplify CategoryOverlay for readability to Simplify CategoryOverlay code for readability.Sep 18 2018, 11:37 AM

Change 460547 abandoned by Niedzielski:
Hygiene: limit CategoryOverlay's inheritance

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

Change 460548 abandoned by Niedzielski:
Hygiene: limit CategoryOverlay's inheritance

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

Change 460575 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: rename Overlay.clearSpinner() to hide

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

Change 460574 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: document CategoryOverlay code w/ function

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

Change 460568 abandoned by Niedzielski:
Hygiene: remove unused Overlay.defaults.fixedHeader

Reason:
The benefit of this patch does not justify the audience and configuration required to review it.

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

Change 461827 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] WIP: separate view logic from CategoryOverlay

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

Change 461827 abandoned by Niedzielski:
WIP: separate view logic from CategoryOverlay

Reason:
Work to be continued in T205126

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

Change 460986 abandoned by Niedzielski:
Fix: a few CategoryOverlay bugs and refactor

Reason:
Work to be continued in T205126

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

So to summarise the activity and output here: We've fixed a few things and identified some bugs over in T205126
Having seen how this went, it feels like we might also want to focus on something more simpler to continue the exploration of our use of Views. @Niedzielski do you feel like T205592 which uses a far simpler component - the Icon - would be just as complicated or do you think that might be worth a go?

@Jdlrobson, what do we hope to learn from T205126 and what approaches do we know that we want to avoid investigating?

I'm not sure how to do effective layout composition with Hogan.js. I'd like to discuss what are options are, even if that ends up being just "don't consider Hogan.js," and what specifically we should explore for T205592. For reference, here's the Hoganfile for Icon.js:

<{{tagName}} class="{{base}} {{base}}-{{glyphPrefix}}-{{name}} {{modifier}} {{#isSmall}}mw-ui-icon-small{{/isSmall}} {{#_rotationClass}}{{_rotationClass}}{{/_rotationClass}} {{additionalClassNames}}"
	{{#id}}id="{{id}}"{{/id}}
	{{#href}}href="{{href}}"{{/href}}
	{{#title}}title="{{title}}"{{/title}}>{{label}}</{{tagName}}>

I would be very interested in defining this pattern but T205126 is too open ended and I'm not sure what we hope to bring back to the team. Any other constraints you had in mind would be very useful in scoping the problem. For example, should jQuery and jQuery event handling be avoided?

Lastly, I'll note that T205592 will require straddling changes across repositories if Icon itself is touched.

@Jdlrobson, totally. I'm very uncertain what I'd do with *Icon.js at the moment.

I think we're good to go here then! Good 'a learnings

Change 460568 restored by Niedzielski:
Hygiene: remove unused Overlay.defaults.fixedHeader

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

Change 460568 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: remove unused Overlay.defaults.fixedHeader

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