New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heatmap/Scatter Plot for discipline incidents by Day & Time #2242

Merged
merged 26 commits into from Dec 12, 2018

Conversation

Projects
None yet
2 participants
@edavidsonsawyer
Copy link
Collaborator

edavidsonsawyer commented Nov 8, 2018

Who is this PR for?

Educators

What problem does this PR fix?

CF #2147, educators would like to see both time and day information on incidents to see if patterns appear.

What does this PR do?

Adds a scatterplot type chart that shows incidents plotted against day and time that also allows users to zoom in on a cluster to see which students are involved.

Screenshot (if adding a client-side feature)

heatmap

Checklists

Which features or pages does this PR touch?

  • Discipline

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Improved specs for existing code in need of better test coverage
  • Manual testing made more sense here

@edavidsonsawyer edavidsonsawyer changed the title Heatmap/Scatter Plot for discipline incidents by Day & Time WIP - Heatmap/Scatter Plot for discipline incidents by Day & Time Nov 8, 2018

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator

edavidsonsawyer commented Nov 8, 2018

Still WIP while I figure out why drag and select isn't working on IE

edavidsonsawyer added some commits Nov 8, 2018

@edavidsonsawyer edavidsonsawyer changed the title WIP - Heatmap/Scatter Plot for discipline incidents by Day & Time Heatmap/Scatter Plot for discipline incidents by Day & Time Nov 9, 2018

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator

edavidsonsawyer commented Nov 9, 2018

@kevinrobinson here's a first pass at a "heatmap" for the discipline dashboards. RIght now zoom is done through a click and drag. That feels a little clunky but it's functional enough to introduce the idea.

I'm not sure what the data looks like in production. If there are so many incidents that it's difficult to distinguish clusters or patterns, we can try some other ways of displaying the incidents (bubbles, moving them apart horizontally, etc.) to improve matters.

export default class DashboardScatterPlot extends React.Component{

//highcharts has trouble combining zoom with a render, so rendering is prevented unless the displayed data changes
shouldComponentUpdate(nextProps) {

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Nov 9, 2018

Collaborator

@kevinrobinson I didn't (just) put this here to bug you. Highcharts seems to have trouble when the selection function is a callback that sets state. Preventing an unnecessary render resolves the issue.

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

👍 This should check categories at the least as well, right? Also, for doing an equality check, I'd suggest _.isEqual(x, y) unless using the hash is important here.

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Nov 14, 2018

@edavidsonsawyer This is awesome! Sorry on the delay here, work on the website has taken more time than I expected, and I've had a harder time switching between all the things last week and this week.

How's this sound for a plan?

  1. I think there's a few things we should improve before shipping, since they'll make it harder for people to understand and have a conversation about what they see. Here's my two cents on what the most important things there might be: a) time labels should flow top>bottom, like a calendar program, b) change the bucketing of time scales so they are at standard intervals like 15m / 30m / 1hr, c) add "gutter" categories for holding "no time" since those aren't visible on the chart right now. I think those little touches might help make the chart more immediately legible to folks, and then they can explore away and we can see what they are thinking about :) If you have other thoughts on this we could chat more about this, or you could suggest something else too!

  2. We ship that, and then you and I can look at some data together ahead of having a conversation with other folks. Then we do a mini design-critique, where you share what you thinking about and what might be a good next improvement, I share my perspective, and then you decide what is the next batch of things to try.

  3. While you're working on the next batch, I can potentially talk with some folks next Monday (next week is a short week!) or if not the week after.

If you want to talk more I could chat late tomorrow afternoon or morning or afternoon Friday too, or feel free to email if you want to talk this out more too, sorry for the late and long reply here :\

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator

edavidsonsawyer commented Nov 14, 2018

Sounds good! I'll be able to get started on that in the next day or so!

@edavidsonsawyer edavidsonsawyer changed the title Heatmap/Scatter Plot for discipline incidents by Day & Time WIP - Heatmap/Scatter Plot for discipline incidents by Day & Time Nov 29, 2018

@kevinrobinson
Copy link
Contributor

kevinrobinson left a comment

@edavidsonsawyer Awesome work! 👍

I looked at some data here too, if you want to videochat and do that sometime later today or Thursday or Friday let me know, that might be helpful for some of the questions around design like colors.

I left some feedback on factoring, but those are more suggestions for the next iteration. I think let's try to ship this now and to do that the final pieces are things that will impact the legibility or interpretability of the data:

  • updating visual colors (lines instead of bands?)
  • showing a student's name on hover
  • adjusting opacity or x-offset so that overlaps are visible visually (or adding heatmap overlay)

If it works for you, we could videochat and look at some data, and then brainstorm on these things and then ship this out! Let me know what works best for you.

@@ -206,7 +237,7 @@ export default class SchoolDisciplineDashboard extends React.Component {
}

studentTableRows(students) {
const filteredStudents = this.filterStudents(students, {shouldFilterSelectedCategory: true});
const filteredStudents = this.state.zoomed ? this.filterToZoomedStudents(students) : this.filterStudents(students, {shouldFilterSelectedCategory: true});

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

This branching to apply a new filter seems like it belongs inside filterStudents, to keep all the filtering in one method, would that make sense?

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Dec 11, 2018

Collaborator

On this one, it's a problem if the logic is in filterStudents because that will also restrict the chart to the currently zoomed students and the chart can no longer be reset. We could work around this, of course, but doing it this way lets us use highcharts' built-in zoom feature.

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 11, 2018

Contributor

Cool, that makes sense! For that case writing this.filteredStudents(students, {includeInvisible: true}) might make that more explicit.

@@ -281,7 +328,8 @@ export default class SchoolDisciplineDashboard extends React.Component {
{value: 'time', label: 'Time'},
{value: 'homeroom_label', label: 'Classroom'},
{value: 'grade', label: 'Grade'},
{value: 'day', label: 'Day'}
{value: 'day', label: 'Day'},
{value: 'scatter', label: 'Day & Time'}

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

What do you think about moving Day & Time to be first, and the default on page load?

@@ -264,6 +295,22 @@ export default class SchoolDisciplineDashboard extends React.Component {
this.setState(initialState());
}

onZoom(highchartsEvent) {

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

Factoring: This seems like a bunch of detail about pixels within the scatterplot, so I'd suggest moving it into that component. For example, DashboardScatterPlot might take onVisibleIdsChanged and then it could handle all the calculations, and then pass the ids back up to this component, which then just calls setState.

But this might better be something tackled when refactoring to re-use the visual component in the profile page.

@@ -95,6 +102,18 @@ export default class SchoolDisciplineDashboard extends React.Component {
return hour;
}

//highcharts will only accept number values as data this returns minutes since the day began.
getincidentTimeAsMinutes(incident) {

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

Factoring: These details seem quite specific to DashboardScatterPlot and maybe like they better fit in that component (eg, defining which minutes are within the school day, what that chart's cutoff for the gutter is). That might be something to take up when refactoring to use in the profile page too, possibly moving these functions into that component.

const props = {school: testSchool()};
const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('DashboardBarChart').length > 0).toEqual(true);

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

should this be DashboardScatterPlot or something else?

const scatterPlotProps = {
...commonProps,
measureText: "Time of Incident",
tooltip: {pointFormat: '<b>{point.name}</b>'},

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

Is the student name available here, or could this be a function that shows the student name instead of the jargony "Series 1"? That's what folks will want to know when they hover.

export default class DashboardScatterPlot extends React.Component{

//highcharts has trouble combining zoom with a render, so rendering is prevented unless the displayed data changes
shouldComponentUpdate(nextProps) {

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

👍 This should check categories at the least as well, right? Also, for doing an equality check, I'd suggest _.isEqual(x, y) unless using the hash is important here.

alignTicks: false
}}
credits={false}
xAxis={{

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

@edavidsonsawyer If you have time anytime this afternoon on Thursday, we can look at some of this data together, it might be insightful!

One thing that happens with the different colored bands here is that the data points with opacity then have different backdrops. Another approach might be to use something more minimal like a calendar program, here's what Google Calendar looks like:

screen shot 2018-12-04 at 10 10 17 am

const incidents = this.getIncidentsFromStudents(students);
const categories = this.sortedDays();
const seriesData = incidents.map(incident => {
const student_id = incident.student_id; //used to identify which points are in view when zoomed

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

If the incident is included here too, then the moment is available for formatting nicely in other places, and the student's name is available for rendering a tooltip.

@@ -429,6 +494,8 @@ function initialState() {
selectedCategory: null,
grade: ALL,
house: ALL,
counselor: ALL
counselor: ALL,
visibleIds: [],

This comment has been minimized.

@kevinrobinson

kevinrobinson Dec 4, 2018

Contributor

I'd suggest visibleStudentIds - it sort of implies disciplineIncidentIds otherwise.

edavidsonsawyer added some commits Dec 11, 2018

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator

edavidsonsawyer commented Dec 12, 2018

@kevinrobinson this last push should make sure all days display If you think this is good to roll out, I'll keep the remaining refactoring for a later day.

@kevinrobinson kevinrobinson changed the title WIP - Heatmap/Scatter Plot for discipline incidents by Day & Time Heatmap/Scatter Plot for discipline incidents by Day & Time Dec 12, 2018

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Dec 12, 2018

@edavidsonsawyer 👍 🚢 !

@kevinrobinson kevinrobinson merged commit 90e7033 into studentinsights:master Dec 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment