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

Reading: Add STAR reading to comprehension, with box and line chart (v2) #2791

Merged
merged 13 commits into from
Mar 26, 2020

Conversation

kevinrobinson
Copy link
Contributor

This is replacing #2778

Who is this PR for?

K5 reading folks

What problem does this PR fix?

STAR wasn't included in the reader profile.

What does this PR do?

Adds in a tab, and a view for STAR reading, refactoring a few pieces along the way. Highlighting uses the same heuristic folks decided on in the class lists (<30th percentile), and the code abstracts that out.

Data is provided by updating reader_profile.rb to re-use some of the StudentProfileChartData class for querying and shape of the data. In the UI, parts <BoxChart /> is factored out into pure structural UI elements so it can be re-used here. And the highlighting heuristic is factored out of the class list feature. No materials are added in this PR.

This PR builds on what was in #2778, but also revises a bit of UI code around the BenchmarkBoxChart, BenchmarkCohortChart and the underlying UI elements and styling. This was driven by some small style changes (eg, decoloring text on cohort charts) that revealed coupling that we don't want anymore now that these aren't UI and design elements aren't just about benchmark reading points.

Screenshot (if adding a client-side feature)

Screen Shot 2020-03-06 at 10 48 53 AM

Screen Shot 2020-03-06 at 10 48 48 AM

Checklists

Which features or pages does this PR touch?

  • Reader profile

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

@kevinrobinson
Copy link
Contributor Author

hrm, blocked on Travis issues at the moment

@kevinrobinson
Copy link
Contributor Author

selfie

@kevinrobinson kevinrobinson merged commit ff98d89 into master Mar 26, 2020
@kevinrobinson kevinrobinson deleted the feature/box-chart-refactoring-wip branch March 26, 2020 17:15
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.

1 participant