Proposed enhancement to dashboard architecture

[Jeremy Kells]

Following on from #400

Now that I've had plenty of time to with/on the dashboard architecture, I want to propose a few enhancements:

1: Naming of files - remove inconsistencies in the file names between the controllers and templates, etc, to make it a little quicker find the relevant code, especially as more dashboards are added.

Also not using 'Province' in the names, but sticking with the default 'District' terminology.

[Ghislain Hachey]

Thanks @jeremy_kells. Personally I fully agree with these enhancement. I don't think @softwords will have any objection.

[Jeremy Kells]

2: I'm not a fan of using MVC/ Razor for the HTML templates of the components - It would be cleaner to fully embrace the SPA architecture and bundle the templates on the client.

I don't want to attempt this at the moment though, probably best considered later, unless there are benefits I am not aware of?

[Ghislain Hachey]

Re 2: I don't currently have a strong opinion on this. I would not want to make some drastic changes here for the time being. @softwords likely has an opinion on this. We can discuss it but let's keep this for another day as it seems like it would be taking on a whole new approach.

[Jeremy Kells]

3: Remove from child components:

this.require["dashboard"] = "^enrolDashboard";

Replacing this with explicit bindings that define how they get data, and how they signal changes back to their parent.

This is more in line with the Component-based application architecture, as specified in the docs.

[Jeremy Kells]

Let me know what you think, and I'll start reorganizing my code so I can submit discrete commits for parts of this. But I think it's worth iterating on what we have before we build out too much on top of it.

[Brian Lewis]

Hi @jeremy_kells @ghachey

1. Yes, please make these changes. I agree too that we keep to the term 'District' in code for District/ State/Province. Remembering that for the UI, this term can and should always be translated via the vocab filter {{'District'|vocab}}

2. This is a far bigger change than just the dashboard components. I think we would put in a lot of effort, and lose functionality rather than gain anything. For just one thing, as it stands, MVC takes care of all the localisation of components using the context path mechanism ( see how the mvc view paths are specially set up in Startup based on the context in Web.config) Bear in mind that every template passed through from the server is added to the template cache on the client anyway. so, for this one : no action.

3. Good point, and I had been thinking the same thing. So the change effectively is:

The 'require' goes from the ComponentOptions class.

An additional binding is added. dashboard : "<"

This means that the class EnrolComponent, which only serves to strongly type the dashboard via the require, can probably go?

Components can assume they have the right class of dashboard object, or in some cases may just declare

dashboard: IDashboard

I'll make 2 issues for points 1 and 3.

[Jeremy Kells]

OK, I think I have a clean local commit for just 1 - I'll split it out and submit PR.

2 - OK, I hadn't looked at the localisation functionality - for the templates themselves, it's just very light template splitting that it is doing.... Agreed, no action for now.

3 - A single binding to dashboard still exposes more of the parent than the client should have access to. And even though it's a one way binding, I'm pretty sure given it's passing an object (by reference) the full parent controller is accessible, still compromising the component model.

Each data value/object should be bound separately, and also each output/callback method.

I was looking at the EnrolComponent class and thinking it was a bit redundant, but I quite like using it as in the image above to hold the common bindings, and allowing you to pass in subclass specific additional bindings.

[Jeremy Kells]

was going to change that 'selectedChild' binding to 'isSelected' and pass a boolean value, as that should be all the child needs to know.

[Jeremy Kells]

Also I think doing this will also allow the removal of the hub - pub/sub, because the individual filter values (i.e. selectedDistrict) will be bound and when they are changed, the binding updates and calls .$onChanges

Need to verify this one though

[Brian Lewis]

Hi @jeremy_kells @ghachey

For me, I take the point of your formal argument about binding, but in a practical sense I think it will lead to generating a lot more code and complexity. I'm not disturbed by the child holding a reference to its dashboard. I think the concern about "visibility" into the dashboard is a bit theoretical, and anyway, you could use a strongly typed reference to the dashboard in the child; so that you can have compile errors if you try to use anything you shouldn't.

With respect, I don't think you have fully thought through the issues... for example, how would the dashboard pass a different value for the binding of isSelected to each client?? Bear in mind too that the client needs to change things in the dashboard - like setting the selectedDistrict when the user clicks on a bar in a chart, or a row in a table for example. The binding approach you propose would end up holding a proliferation of callbacks to manage this, and the syntax for using the callback would actually be more complex than what it is now. (& bindings)

We have a model now that handles all the inter-component communication; it's nice and simple, it works, and it's all encapsulated in the bases classes. Can I suggest then we just do the tweak in issue #435, then move on to building some components with it. I can swap that task back to me if you prefer.

I'm sure that when you get your "fingers in the clay" there may be some practical refinements we could consider, but given the capabilities that are there, I don't believe we need any significant changes to the design.

[Jeremy Kells]

Debating binding vs exposing a components internals will likely be as useful as debating OO Encapsulation and strong interfaces (including typescript vs javascript) - it's all been said before.

As far as I know, you would have to create a detailed typescript interface that would be every bit as explicit as actually doing the binding to get any compile time help from typescript. And this should be done seperately for each childComponent (they require different data/callbacks from the parent). This could almost be a valid approach, but given that the angular js event model is also being compromised by this (i.e. as far as I can tell it is this lack of bindings why added the dashboard.hub pub/sub (and the option-change binding) messaging is needed). With bindings in place, the dashboard will update the child when the data changes, and any special handling (update chart filters) can be done via the $onChanges event directly. So we should be able to strip out much of the current plumbing for propagating updates, and rely on the angular framework to do it.

Yes, there is a lot more bindings, and yes you do have to pass callbacks - so far the & bindings seem simple and work well (quirky call syntax where you pass a JS object to map to the parameters aside) - have you encountered limitations with them before?

isSelected is a standard pattern for this type of design - it just requires moving the logic that compares the parent.selectedChild property with the child.Id to the parent - typically into the template. Something like this:

  <dashboard-bar-chart class="dashboard-wrapper height2 width3" 
                       is-selected="vm.isSelectedChild('EnrolmentByAuthority')"
                       toggle-selection="vm.toggleSelection('EnrolmentByAuthority')"

As you can see the child IDs are created on the parent and passed down. I took a quick look at this but it's complicated by the razor templating, so best to leave the child defined ids and passing the .selectedChild for the child to check

Anyway, let's look as some code, as the advantages of this approach.

I've created PR #216 to give you an idea of how I think we should define the interface - it's not ready to merge, and is not complete but it has:

• DashboardChild base class more or less unchanged - still using dashboard reference
• All extended logic in subclasses of this using bindings.
• Moved from specific <enrolment-district-chart> component to generic <dashboard-bar-chart>
• • This allowed "Enrolment by Authority" chart to just by adding the markup to Enrolment.cshtml, and adding one more property to the EnrolDashboard Controller

Dashboard.BarChart.Component.ts (one line added)

      this.grpAuthorityCode = this.dimAuthorityCode.group().reduceSum(d => d.EnrolM + d.EnrolF)

Enrolment.cshtml

        <dashboard-bar-chart class="dashboard-wrapper height2 width3"
                       dashboard-ready="vm.dashboardReady"
                       selected-child="vm.selectedChild"
                       option-change="vm.optionChange"

                       heading="Enrolment by Authority"
                       chart-cf-dimension="vm.dimAuthorityCode"
                       chart-cf-group="vm.grpAuthorityCode"
                       data-filtered-on="vm.options.selectedYear"
                       highlighted-row="vm.options.selectedAuthority"
                       selection-callback="vm.options.toggleSelectedAuthority(key)" />

Dropping this in and restarting and the new chart had all it's data, and was displaying correctly - just some of the events are not propagating - it's time to untangle the $onChanges (and probably all of the DashboardChild base class) code next.

This is where the benefits of this (component based) Design start to come through - there is a nice separation between the dashboard fetching the data (and doing overall layout), and the children just taking what they are given and displaying it (and wiring up callbacks).
Yes, there is a bit of design effort now, but it will enable new dashboards to be created much faster when built on this base.