Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-76349

Policy around peer review and granularity of mustache templates

XMLWordPrintable

    • Icon: Improvement Improvement
    • Resolution: Deferred
    • Icon: Minor Minor
    • None
    • Future Dev
    • None

      In older code using renderers and renderables I think there was a lot more discipline and thought given to the best granularity of each method. If there was a button method then other renderers would use that, and if a render method got too big then this would routinely be picked up in review as a smell and get broken down into better sized methods which ideally did a single thing and did that one thing well.

      With the swap to templates I think a lot of the discipline around this has disappeared. 

      There are a couple concerns:

      1) Difficulty in overriding them

      The point of templates is twofold: to make them easier to author, and easier to override. I would argue the second point has generally gotten worse with templates.

      A simple example might be a button or a notification. Lets say a client has some particular requirement an wants all notifications styled in a particular way, and that the best way to do that is by adding some markup to the notification rather than a trivial css. Previously you would override notification() in the renderer, and now in theory you can do that by overriding /lib/templates/notification_base.mustache

      But a quick scan shows there is a heap of places where other mustache template do not call this partial and just directly output the duplicated bootstrap styles:

      content/export/module_index.mustache:71:    <div class="alert alert-info alert-block">
      content/export/course_index.mustache:76:    <div class="alert alert-info alert-block">
      content/export/course_summary.mustache:63:    <div class="alert alert-info alert-block">
      external_content_banner.mustache:39:<div class="alert alert-secondary alert-block fade in">
      local/notification/cta.mustache:52:<div class="cta alert alert-primary alert-block fade in {{ extraclasses }}" {{# region }}data-region="{{ region }}"{{/ region}}>

      So if you are a themer this makes the job exponentially harder.

       

      2) Consistency across the platform.

      In the example above, are the manually crafted notifications all on par when it comes to accessibility? 

      But in most cases there is no partial to begin with, and there it lots of code duplicated in templates that could be refactored.

       

      It is hard to say exactly what the action or policy suggestion is, but I think at minimum this should be something considered at peer review time.

      A good rule of thumb could be that the most common components in the component library are refactored into mustache templates partials. Even if a partial exists there may still be valid times to not use it, we don't need 100% adherence for the same of consistency over the best UX on a particular page, but that can be easily justified in the tracker feedback.

       

            Unassigned Unassigned
            brendanheywood Brendan Heywood
            Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

              Created:
              Updated:
              Resolved:

                Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.