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

Structure refactoring/incidence rates #797

Merged
merged 6 commits into from
Aug 2, 2018

Conversation

johnSamilin
Copy link
Contributor

@johnSamilin johnSamilin commented Jul 30, 2018

part of #633

ASaltykov added 4 commits July 20, 2018 13:25
# Conflicts:
#	js/components/ir-manager.js
#	js/main.js
#	js/modules/iranalysis/components/editor.js
#	js/modules/iranalysis/components/report.js
@chrisknoll
Copy link
Collaborator

@johnSamilin ,

Something changed in the Incidence Rate API where when we used to get the report data back from this call:

IRAnalysisService.getReport(source.info().executionInfo.id.analysisId, source.source.sourceKey, this.selectedTarget(), this.selectedOutcome()).then((report) => {

But now the resulting data structure is soemthing resembling an XHR request:

image

The data is buried somewhere under the 'data' field, but the api is just supposed to return the data, not the low level details of the ajax request. Can you change this to have the previous behavior?

@johnSamilin
Copy link
Contributor Author

@chrisknoll sure, fixed it

Fixed css path to report stylesheet found in cohort builder.
@chrisknoll
Copy link
Collaborator

@johnSamilin , I pushed up some fixes:

  1. There was a reference to a css style found in another module so that was just an update.
  2. More serious: pure computeds that need to access the view model (formerly, this was handled via a 'self' reference) were having 'this' result in 'undefined' when triggered by an asynchronous update in the ui (ie: one of the observables notified a change). Converting the function declaration to arrow function allowed the 'this' ref to be resolved to the view model, but I wanted to call this out for future changes to make sure that functions that reference view models via 'this' are probably bound. I like what we're doing with the refactoring, but this element of ES6 classes is making me a bit unhappy.

If you don't see any problems with these changes, I can approve and merge it in.

@johnSamilin
Copy link
Contributor Author

Yes, seems like I just missed those two functions. When I analyse the code of the component, I convert the callbacks from regular functions to arrow ones to preserve its context.
I'm not quite sure that I understood the second point correctly, you wouldn't want to use arrow functions or to bind class methods to this?

@chrisknoll
Copy link
Collaborator

chrisknoll commented Aug 2, 2018

I'm not quite sure that I understood the second point correctly, you wouldn't want to use arrow functions or to bind class methods to this?

No, the arrow-function style is fine with me. It's just that we need to make sure that we are using arrow functions where the function might be called async and 'this' becomes undefined. So, use arrow functions because they handle the 'this' binding to the view model automatically.

If you have no objections, I will merge this PR in.

@chrisknoll chrisknoll merged commit a9c252d into master Aug 2, 2018
@chrisknoll chrisknoll deleted the structure-refactoring/incidence-rates branch August 2, 2018 17:51
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.

2 participants