Page MenuHomePhabricator

Spike: Do a code review session on Skin.js
Closed, ResolvedPublic

Description

Skin.js has become large and appears to be operating more like an Application then a skin. We may want to revisit this file, pull things out of it, move it out of mobile.startup and maybe even move some of it to skins.minerva.script.

Let's chat about the code as a group and see how we can improve this part of the codebase.

Outcome: A plan for refactoring (if any)
Inform and update T173454

Duration: 2hrs

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJun 27 2016, 3:19 PM
Jhernandez triaged this task as High priority.Jun 29 2016, 5:15 PM
Jhernandez added a project: Technical-Debt.
Jhernandez moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson lowered the priority of this task from High to Normal.Feb 6 2017, 8:09 PM

This is still relevant. I'm hoping we can get round to this soon... I fear our PHP code needs some love first.

Jdlrobson updated the task description. (Show Details)Aug 16 2017, 7:49 PM
Jdlrobson closed this task as Resolved.Sep 6 2017, 6:40 PM
Jdlrobson claimed this task.

Outputs:

There was an interesting conversation started by Piotr about our use of comments in our projects. We came to the conclusion that inline comments are usually only useful if they describe "why" things are the way they are.

e.g. https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.tablet.scripts/toc.js#L27
we felt the comment should describe why a table of contents would be there - not explain the line that followed.

I think this was a useful session. We only spent an hour here, but I think the outputs should be to focus on the 3 tasks mentioned above.