Skip to content
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

Prison stay lengths section with bar charts #333

Merged
merged 27 commits into from
Feb 4, 2021

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented Feb 2, 2021

Description of the change

Ports over another chart component, the BarChartTrellis, to implement the prison stay lengths section. This was really only a UI change since this section uses the same data handling logic implemented in #332 .

Screen Shot 2021-02-02 at 3 11 20 PM

The BarChartTrellis component is simplified compared to its v1 counterpart, because the layout is different (single column) and it doesn't really need the Semiotic FacetController (the v1 component probably didn't really need that controller either, we live and learn). This in turn let me remove some special case handling from the ResponsiveTooltipController (specifically the render prop, which was only needed to support FacetController). Some assorted type changes and cleanup fell out of that.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #303, closes #304

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@coveralls
Copy link

coveralls commented Feb 3, 2021

Pull Request Test Coverage Report for Build 535119573

  • 47 of 54 (87.04%) changed or added relevant lines in 11 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 67.753%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotlight-client/src/charts/utils.ts 5 6 83.33%
spotlight-client/src/VizPrisonStayLengths/VizPrisonStayLengths.tsx 14 17 82.35%
spotlight-client/src/charts/ResponsiveTooltipController.tsx 4 7 57.14%
Files with Coverage Reduction New Missed Lines %
spotlight-client/src/charts/ResponsiveTooltipController.tsx 3 46.15%
Totals Coverage Status
Change from base Build 534607744: 0.3%
Covered Lines: 1911
Relevant Lines: 2728

💛 - Coveralls

@macfarlandian
Copy link
Collaborator Author

realized after opening the PR that I had broken highlighting with my type changes ... fixed it and added a hook to consolidate some type checking for highlight states to try and harmonize things a bit more

Base automatically changed from ian/301-bubble to master February 3, 2021 17:21
jovergaag
jovergaag previously approved these changes Feb 3, 2021
Copy link

@jovergaag jovergaag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📽️

@jovergaag jovergaag dismissed their stale review February 3, 2021 20:58

I meant to approve Daniela's PR - un approving since I haven't finished looking at this yet :)

@macfarlandian
Copy link
Collaborator Author

🎢

Copy link

@daschi daschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏖️

});
});

test("loading", () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if this would also be the place to test that we see the NoMetricData component when metric has an error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yeah I haven't been doing that due in part to laziness (error handling is tested in the Metric model but not in the UI) but also because the error handling is still kind of a placeholder ... there is a ticket #327 to flesh it out, I made a note there about the existing test coverage because it may depend on what the real designs are

@macfarlandian macfarlandian merged commit f3d8e8d into master Feb 4, 2021
@macfarlandian macfarlandian deleted the ian/303-304-stay-lengths branch February 4, 2021 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prison stay lengths viz Faceted bar charts component
4 participants