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

Student Profile Heatmap #2349

Merged
merged 11 commits into from Jan 11, 2019

Conversation

Projects
None yet
2 participants
@edavidsonsawyer
Copy link
Collaborator

edavidsonsawyer commented Jan 10, 2019

Who is this PR for?

Educators

What problem does this PR fix?

Users of the discipline dashboard can look at a heatmap of aggregated discipline incidents. It may be useful to see a heatmap for an individual student to find patterns in discipline incidents.

What does this PR do?

Adds a heatmap of a student's individual discipline incidents to the student profile in the Incident History section

Screenshot (if adding a client-side feature)

heatmap_student

Checklists

Which features or pages does this PR touch?

  • Student Profile
  • Discipline

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

  • Included specs for changes
  • Manual testing made more sense here
@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator Author

edavidsonsawyer commented Jan 10, 2019

@kevinrobinson here's the heatmap adapted slightly for the student profile. One improvement that we'll need to make is to set the school hours in one location and import into this and the dashboard component so the times don't get out of sync. I want to check in with you first, though, to find out whether there are other components that might use a feature like that so it can be done all at once.

@kevinrobinson
Copy link
Contributor

kevinrobinson left a comment

@edavidsonsawyer This looks awesome! I left a few small suggestions, and if these seem good we could deploy this tomorrow! 👍

}
}
>
<div

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 10, 2019

Contributor

This snapshot isn't testing much since HighchartsWrapper doesn't render much itself directly. If you add in jest.mock('../components/HighchartsWrapper', () => 'highcharts-wrapper'); before the it calls in the test, that will mock the component, and then snapshot test will verify that <HighchartsWrapper /> is called with the same arguments over time.

If you add that in and then run yarn jest app/assets/javascripts/student_profile/IncidentHeatmap.test.js you can see the diff and how the snapshot becomes more useful in checking that the way the component is called stays the same over time.

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Jan 11, 2019

Author Collaborator

Ah, thanks for catching that!

@@ -20,7 +20,7 @@ export default class DashboardScatterPlot extends React.Component{
}

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 10, 2019

Contributor

This seems great! In this pull request, can you also move this file into components since we'll re-use it across the school_administrator_dashboard and student_profile folders?

super(props);
}

getincidentTimeAsMinutes(incident) {

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 10, 2019

Contributor

This works but getincidentTimeAsMinutes looks like it's copy-pasted from SchoolDisciplineDashboard. Could you move it (and areMinutesWithinSchoolHours) to a plain function that DashboardScatterPlot exports? That way both callers can use that same function, and maybe that helps with what you were asking about re: "setting the school hours in one location?"

incidentCard={incident}
/>
))}
<div className="IncidentHistory" style={{display: 'flex'}}>

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 10, 2019

Contributor

This class name doesn't look like it's used for anything, can we remove it? In general, let's use names to label the root element inside a component (with the same name as the component), or for things that need proper CSS, for testing or other JS interop.

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator Author

edavidsonsawyer commented Jan 11, 2019

@kevinrobinson thanks for the comments! They all sound good to me and have all been implemented

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Jan 11, 2019

@edavidsonsawyer awesome, 🚢 ing now! 👍 ⚡️

@kevinrobinson kevinrobinson merged commit 3987484 into studentinsights:master Jan 11, 2019

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