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

Hide scatterplot for large schools #2362

Merged
merged 2 commits into from Jan 22, 2019

Conversation

Projects
None yet
2 participants
@edavidsonsawyer
Copy link
Collaborator

edavidsonsawyer commented Jan 21, 2019

Who is this PR for?

Educators

What problem does this PR fix?

For large schools, the scatterplot in the discipline dashboard has too many datapoints to show useful information.

What does this PR do?

Removes the scatterplot feature when there are more than 500 discipline incidents to display when first loading

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

This comment has been minimized.

Copy link
Collaborator Author

edavidsonsawyer commented Jan 21, 2019

@kevinrobinson here's the simpler version of disabling the scatter plot for large schools. I'm not sure what's up with the build error so I'll try to look into that later on today.

const el = testEl(props);
const dash = mount(elWrappedInContext(el, context));
expect(dash.find('DisciplineScatterPlot').length > 0).toEqual(true);
});

it('does not render a scatter plot when there are more than 500 incidents', () => {

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 22, 2019

Contributor

nice!

this.setState({selectedChart: 'incident_location', scatterPlotAvailable: false});
}
}

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 22, 2019

Contributor

Can you say more about why this is here, rather than computed in render? I don't follow why it's using the lifecycle hook.

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Jan 22, 2019

Author Collaborator

It's here because the state has to change (from default scatter to some alternative), which can't be done in render. I could probably set the initial state conditionally in the constructor, but it seemed cleaner to just use getIncidentsFromStudents in the mounted component. It shouldn't be much of a performance issue since we cache the result of that function with memoizer.

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 22, 2019

Contributor

ah, thanks! Got it, it's because this is set in initialState in the constructor, that's super helpful. Using the lifecycle hook implies that it's because of something related to the DOM or outside of React, and then leads to double-rendering on load.

Yeah, the better way to do this is describe this explicitly in the constructor, and then let initialState decide what selectedChart should be based on those props. Here that would involve factoring out the incident and filtering functions to be plain functions (rather than relying on this.state and this.props) and that seems a larger change than this is worth.

So if you tested this and it works without a visible jump, let me know and let's 🚢 as is. And thanks for explaining and helping me learn! 👍

This comment has been minimized.

@edavidsonsawyer

edavidsonsawyer Jan 22, 2019

Author Collaborator

Great! I have tested this locally with schools above and below the 500 student threshold, and in the former case the scatter plot is cleanly removed. I would like to double check in production just to be sure the threshold is appropriate, but that can probably come from feedback from users.

This comment has been minimized.

@kevinrobinson

kevinrobinson Jan 22, 2019

Contributor

@edavidsonsawyer awesome, thanks! 🚢 ing now and I'll verify as well when it's out.

@kevinrobinson kevinrobinson merged commit 90e396e into studentinsights:master Jan 22, 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 22, 2019

@edavidsonsawyer looks good! 👍 It looks like there's a separate issue with SHS, which I'd best is from the call to latest_note (since a larger MS school in New Bedford with less notes loads just fine). I'll add that to the list of things to get this functional enough for Somerville general release.

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