Eliminate dead and redundant code from enrolment dashboard components

[Brian Lewis]

@jeremy_kells this will be up in the next hour

[Jeremy Kells]

It's not so much the redundant code that is frustrating, it's the lack of an abstraction - and that is why all the copypasta is needed.

Eliminating the redundant code is a start and will illuminate just how little is different between the components, but will not help you when you want to change some minor thing about all of them in 6 months time, and you have to go through every single copy pasted component.

The masonry.js layout is not working well either - did you have a particular layout in mind?
I can look if you want.

[Brian Lewis]

chore(dashboards): refactor, cleanup redundant code; closes #446

→ <<cset ed99901c8574>>

[Brian Lewis]

@jeremy_kells this is done now ...

[Brian Lewis]

@jeremy_kells ( @ghachey )

I saw your comment above last night and have had a think about it overnight.

For a broader picture, the components you have built so far are very rudimentary - and don't have much point of difference.

But I am hoping that as these get fleshed out into richer objects, that they become more differentiated from each other, because of the actual meaning of the data. In particular, I'd like us to make use of the Selected state to offer some drill down into the data somehow - as a first example, see what I have done with the LevelByAge, which has a condensed and expanded table of data.

So for those reasons I am happy for them to sit as objects in their own right - albeit derived from a subclass.

You haven't used the the crosstabbing yet as shown in LevelByAge, which use the xFilter object xReduce and xReduceTotal methods to create a crossfilter group where the value is an object, not a scalar.

These can then be rendered as a table; and I think at this lower level there is scope for a further abstraction; ie a crosstab widget whose job is to render the groupings created by the dashboard component. This would be implemented as a component, that would be referenced in the template of the dashboardcomponent (note: not in the template of the dashboard).

So what I propose (and I will write up issues) is that -- you take e.g. your schooltype component, and split it into columns by district. ie use the xFilter.xReduce methods to do this.

-- then, using the models in levelbyage, build an xTable component as described in this note and use it in the template of the dashboard component.

[Jeremy Kells]

The main reason for creating abstractions (and using declarative ones in particular) is to separate the "What do I want to do" from "How do I do it"

What I want to do is:

• Display this data (2D grid, e.g. EnrollmentByDistrict)

• Displayed as a PieChart (or BarChart/Table/etc)

The cleanest way to do this is in the Dashboard Component's template file:

<pie-chart data="vm.EnrollmentByDistrict()" />    <!--(yes I am skipping/simplifying bindings)-->

Then say we want to change this to a bar-chart

<bar-chart data="vm.EnrollmentByDistrict()" />    

Or Change the dataset:

<bar-chart data="vm.EnrollmentByIsland()" />    

There is a clean separation from the implementation details of how a bar chart component instantiates itself, or anything else below this level. Also the 'chrome', (titles, background, border, fonts, etc) around the component will be defined and consistent across all components.

I've created a pull request with code for #444 - There are two commits, first one uses a bar chart, then the second converts this to a Pie-Chart - Using the lower abstractions you created to wrap the dc.js (non angularjs) components. Three changes were required:

• dcCharts.ts - Below the abstraction, but just adding a missing call, will now benefit all PieCharts built on this (abstraction doesn't leak)
• EnrolmentAuthorityChart.chtml - Declarative statement about What to do - use PieChart instead of BarChart - (Perfect, if only it was one abstraction level higher!)
• EnrolmentAuthorityChart.component.ts - This is detail I do not want to think about when working on the level of What do I want to show - this is a leak in the abstraction. Now it's likely pass through from inside the dc.js codebase, but should be dealt with below the abstraction(s) - i.e. in dcCharts.ts

There are a few other little things I'll comment on in the PR #229 as there are other abstractions breaking down.

Briefly on your other points:

Yes so far these may be simpler widgets- but you'll probably find that most (80%+ (random statistic)) components will fit into this category. 80/20 rule, optimize for these components. Make the easy stuff easy, and the harder stuff possible - because just creating a set higher level abstractions for the most common widgets doesn't stop you from creating more complex, custom sub-classed widgets.

Ideally though, many more complex widgets can be deconstructed into simpler components, which hopefully are these simpler widgets.

I'll take a look at the #449 next

[Jeremy Kells]

if we had simple, composable components, this change (#444) would be reduced to creating a wrapper for them:

<enrolment-by-authority-or-govt
  authority-data="vm.authorityData"
  authority-govt-data="vm.authorityGovtData" 
/>

translates to:

<div>
    <bar-chart ng-if="vm.isSelected()"  data="vm.authorityData" />
    <bar-chart ng-if="!vm.isSelected()" data="vm.authorityGovtData" />
</div>

[Jeremy Kells]

@softwords @Ghislain Hachey

Also what do you want to do with the masonry.js layout issues mentioned earlier?

[Ghislain Hachey]

@jeremy_kells regarding masonry.js layout issues we will do nothing and spend no resources on them for the moment. They will be dealt with at a later point in time (#454). This affects nobody here as nobody has giant screens with very large resolutions.

Let's just all focus on getting some dashboard components in there. Once you have looked at @softwords 's recommended issues to explore various features of the architecture please we need to look at building a few nice looking dashboard with several components for each of the following in order of priority:

• Schools
• Individual School
• Teachers
• Individual Teacher
• Students
• School Accreditations
• Exams
• Individual exam ?!

[Jeremy Kells]

@ghachey I haven't looked at the masonry layout behaviour - though you don't actually need a large screen to see it's not ideal. Will leave it.

Working on @softwords's suggested #449 - And aside from a few teething issues (PR #234) this looks like a good base to build on.

[Ghislain Hachey]

Thanks @jeremy_kells . Yeah it's pass the good enough layout behavior for the time being from what I have seen. We will certainly revisit this a little further down the line probably when we do a final push to cleanup all the UI and fully move everything to material. Certainly open to suggestion at that point.

@softwords will be in and out of his office today but he'll be merging you're PRs as soon as possible. Hopefully this does not hold you back too much.