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

Fix stats summary table time of year bugs #247

Merged
merged 19 commits into from
Dec 8, 2018
Merged

Conversation

rod-glover
Copy link
Contributor

@rod-glover rod-glover commented Dec 6, 2018

Resolves #215
Resolves #193
Resolves #176

This PR's main effect is

  • Creates a new component StatisticalSummaryTable. This component is extracted from SingleDataController and significantly refactored to fix the time of year bug and conform to React 16+ lifecycle usage.
  • SingleDataController gets much simpler -- much of it is gone into StatisticalSummaryTable. The diff is ugly though.

Other effects are:

  • Convert DualDataController to native class extension style
  • Remove unnecessary dependency on DataControllerMixin from DualDataController
  • Make ce-backend function signatures uniform
  • Comments and clarifications here and there

@rod-glover rod-glover force-pushed the stats-summary-time-bugs branch from 08bbad8 to 0305b53 Compare December 6, 2018 23:24
@rod-glover
Copy link
Contributor Author

Demo

Copy link
Contributor

@corviday corviday left a comment

Choose a reason for hiding this comment

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

Looks good, works well. I'm very glad to finally have the stats table encapsulated, good job.

// This mixin is no longer relevant, since it is only used in the deprecated
// component MotiDataController. Dependence on it has been removed from all other
// data controllers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yay! Finally escaped the clutches of the mixin!

function displayError(error, displayMethod) {
// Used to display any error (via `displayMethod`) generated in the
// process of showing a graph or table, so it handles networking
export function errorMessage(error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes seem familiar; does this branch need to be rebased?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebasing

@rod-glover rod-glover force-pushed the stats-summary-time-bugs branch from 13a1521 to 91a21cb Compare December 8, 2018 00:37
@rod-glover
Copy link
Contributor Author

Since rebase proceeded without manual merging, and follow-up change is trivial, I'm going to merge this without further review. It all works, still.

@rod-glover rod-glover merged commit ee44939 into master Dec 8, 2018
@rod-glover rod-glover deleted the stats-summary-time-bugs branch December 8, 2018 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants