Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Category Overlay review

Improving category overlay

Homework: Everyone to review the CategoryOverlay code (https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.categories.overlays/CategoryOverlay.js) and related CategoryGateway (but not CategoryAddOverlay.js) and think about how they would write it given a magical imaginary architecture (e.g. if we're using React). On 9th April we will talk about this code and pain points and share what we've learned. The goal would be to identify a rewrite task with clear goals and an outline of first steps to a refactor. …

What is the category overlay

The category overlay is a beta feature on mobile wikipedia that shows a full-screen overlay of page categories. The categories are divided into two lists "content based" and "organizational" under a set of tabs. Clicking the tab switches the list, and clicking a category brings you to the category page.

What does it do?

The category overlay does the following things:

  1. Registers with OverlayManager to close/open/render the overlay.
  2. Invokes CategoryGateway to fetch the list of categories
  3. Extends Overlay in order to populate an ovelay instance with specific content
  4. Creates tabs to switch between category lists
  5. Creates the category lists
  6. Invokes infiniteScroll to lazy-load more elements from CategoryGateway
  7. Manages the inifiteScroll loading & behaviour

Can we identify discreet components based on this functionality?

  1. The overlay itself
  2. Tabs
  3. List + list items (for what is an empty list?)
  4. Infinite Scroller

In React land, the component tree could be expressed like this:

<Overlay>
    <Tabs> 
        <TabHeaders labels="['content-based', 'organizational']"/>
        <TabContent>
            <Panel for="activeTab()" with="infinteScroll()">
                <CategoryList>
                    <CategoryListItem/>
                </CategoryList>
            </Panel>
        </TabContent>
    </Tabs>
</Overlay>

Points of discussion.

In the component tree above, there is only one content panel, but two tabs. This assumes the ability to switch the DOM of the <Panel> component based on the active-state of the tab headers.

Infinite scrolling - How is scrolling handled when there are actually two different scrollable lists? When you scroll the "content-based" category list, does the "organizational" category list get updated too? Or, should these be two seperate scrollers? Does that mean two different API requests?

Misc - The CategoryOvelay defines a category-added event but doesn't actually use it, can we better document these kinds of shared events.

What about the gateway?

  • First of all, CategoriesGateway extends SearchGateway? why?

    • Can't find any methods in CategoryGateway that are reused from SearchGateway
    • Doesn't seem to take advantage of the _cache function in SearchGateway
  • Why is this is inheriting from SearchGateway and then extending it's prototype?

    • Oh this is why because OO.inheritClass overrides the prototype

    Beware: This redefines the prototype, call before setting your prototypes.

    • If seach gatway is needed, why not just call SearchGateway.apply instead of CategoryGateway.parent.apply in the constructor? and then use OO.mfExtend like everywhere else?
  • The use of util.extend to add a parameter in the search method is confusing, because util.extend is so frequently used to extend classes.

  • I see a then, but no catch

  • I see a jQuery.Deferred in the docs, is it really?


Conclusions

Similaries with other components

Infinite scrollers

The category overlay manages the inifinite scroller in a very similar way to Special:Uploads (and maybe others). Components have to manually create and decide when to show/hide the spinner, seems like this could be managed by the infiniteScroller itself. Triggering a reload also has to be invoked manually by the component, can this be managed by the infinite scroller by triggering an event instead?

Gateway behaviour

The CategoryGateway processes data in a similar way as the SearchGateway. It has to check if the response contained data.query.pages (or something) or if it's empty or contains an error. The Gateways should probably handle errors and serve the components just the data they need to do their thing, so instead of data.query.pages the gateway should return just pages or an empty array (or some other convention if the response is empty, return false maybe?)

Can we abstract the common handling into a generic Gateway component? seems like request caching could be utilized by many components, as well as error handling and continue handling. Something like a .next() method in the case of canContinue would be nice.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.