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

Profile Heatmap Additions #2353

Merged
merged 6 commits into from Jan 17, 2019

Conversation

Projects
None yet
2 participants
@edavidsonsawyer
Copy link
Collaborator

edavidsonsawyer commented Jan 16, 2019

Who is this PR for?

Educators

What does this PR do?

Makes the incident history and profile heatmap more prominent in the student profile page and makes heatmap reflect the clean slate filter. Also improves the heatmap tooltip and restores a missing spec for the heatmap component.

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?

  • Improved specs for existing code in need of better test coverage
  • Manual testing made more sense here
@kevinrobinson
Copy link
Contributor

kevinrobinson left a comment

@edavidsonsawyer awesome, great catch! 👍 🐱 I think the great way you wrote the tooltip prop might work so you don't even have to add the studentChart branching to scatterplot component, let me know what you think.

Also, the checkboxes on the pull request description are blank, is there more to do here or is this just about ready to 🚢 ?

EDIt: oh sorry if I jumped the gun, just got excited :)

@@ -134,7 +134,7 @@ export default class LightBehaviorDetails extends React.Component {
))}
</div>
<div style={{width: '50%'}}>
<IncidentHeatmap incidents={this.props.disciplineIncidents}/>
<IncidentHeatmap incidents={filteredDisciplineIncidents}/>

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 16, 2019

Contributor

👍 😄

@@ -110,6 +114,7 @@ DisciplineScatterPlot.propTypes = {
seriesData: PropTypes.array.isRequired, // array of JSON event objects.
titleText: PropTypes.string, //discipline dashboard makes its own title
measureText: PropTypes.string.isRequired,
studentChart: PropTypes.bool.isRequired, //flag to indicate the chart is displaying individual student data

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 16, 2019

Contributor

Can you accomplish this with the current API, by passing different tooltips?

This change would modify the generic scatterplot component to now have code that's specific to the profile page and dashboard page. It looks like you could have them pass in the tooltip they need, and then this component stays generic.

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Jan 16, 2019

Author Collaborator

@kevinrobinson you're fast on the draw! I was actually looking at this and I agree that it's fine to pass a function here instead of a flag. At the time I thought there might be other reasons to distinguish behavior based on whether it was for a student or school.

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 16, 2019

Contributor

👍 also the test failure https://travis-ci.org/studentinsights/studentinsights/builds/480544138?utm_source=github_status&utm_medium=notification seems wacky, you can try just restarting it and if it still fails ping me and I can help debug:

image

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Jan 16, 2019

Author Collaborator

@kevinrobinson the old turn-it-off-turn-it-on-again trick does it again

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 17, 2019

Contributor

cool, yeah there's got to be a race condition in that spec related to testing scrubbing data sent to Rollbar, but not sure where. So if it's happening .05% of the time then not so bad but yeah flaky tests are no bueno...

edavidsonsawyer added some commits Jan 16, 2019

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Jan 17, 2019

@edavidsonsawyer awesome, 🚢 ing!

@kevinrobinson kevinrobinson merged commit bb53cb7 into studentinsights:master Jan 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Jan 17, 2019

@edavidsonsawyer Ah yeah I just realized the time range is probably counter-intuitive for the chart here, and the copy might be confusing. It says "this page only includes one year of data by default" since that was made right at the start of the school year. So regardless of the time of year, it includes everything for the current school year, and for the previous school year. This makes more sense for the feed of text, but less sense for something aggregating the whole time period like the chart.

One idea is, maybe we add another filter on top of the "clean slate" filtering, so that the chart only gets events in the current school year, and then make that explicit in the title of the chart.

What do you think? We should probably also clarify the "clean slate" copy too.

@edavidsonsawyer

This comment has been minimized.

Copy link
Collaborator Author

edavidsonsawyer commented Jan 17, 2019

@kevinrobinson I'm not sure. When I look at the screen I think I'd expect the chart reflects the incidents I see in the feed. I do see why it might not be meaningful to look for patterns across a year + school year to date. Do you think it might make sense to change the "clean slate" filter to just begin at the school year beginning?

@kevinrobinson

This comment has been minimized.

Copy link
Contributor

kevinrobinson commented Jan 18, 2019

@edavidsonsawyer I think there's three good reasons to update the chart to show the current school year. First, I think the strong default assumption for everything is "this school year" unless there are places where we really explicitly call that out (eg, past 45 days). I don't think anyone who's trying to dig into discipline incidents is going to read the "clean slate" copy and notice what it says :) Second, students' schedules often change significantly from year to year; for students transitioning schools (say 8th grade to HS) it will be completely different. This means showing two years will hide any patterns that there might be related to the schedule or school day in the current year. Third, patterns from the current school year are the only thing that's potentially actionable.

So my vote would be to make the chart reflect the school year and label that explicitly like 2018-2019 school year.

Separately, it also seems like a good idea to clarify the clean slate message so it's clearer that it's the current and previous school year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment