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

Json Instruments V1 - WIP #3136

Closed
wants to merge 247 commits into from
Closed

Conversation

jacobpenny
Copy link
Contributor

WIP PR to facilitate conversation

@jacobpenny
Copy link
Contributor Author

@driusan Addressed your comments. Still need to fix one test and do javascript linting though.

@jacobpenny
Copy link
Contributor Author

The React components are in need of some optimization. For most instruments it isn't an issue but while testing one of our instruments (which contains 704 elements) we noticed significant slow-downs, mostly when editing a text box. As far as I can tell there are two causes:

  1. Wasteful re-rendering
  2. Evaluation of RequireResponse, DisplayIf, and Score Fields whenever there is user input.

Here are some ideas for optimizing:

  1. Use react-addons-perf's printWasted method to see which elements are being needlessly re-rendered. (https://facebook.github.io/react/docs/perf.html#printwasted). Overriding the lifecycle function shouldComponentUpdate can avoid these wasteful re-renders for these elements.

  2. The two functions annotateElements and filterElements, called here and here, could be combined to reduce the number of iterations through the elements.

  3. Avoid evaluating RequireResponse until the user tries to submit.

  4. For text input fields, instead of triggering an update onUserInput (i.e. every key stroke), only trigger the update onBlur. This is tricky because the text components would need to track their own state (i.e. no longer be controlled inputs).

@ZainVirani ZainVirani self-assigned this Sep 18, 2017
@ridz1208 ridz1208 changed the base branch from 17.1-dev to major October 21, 2017 17:39
@driusan driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Dec 19, 2017
@driusan
Copy link
Collaborator

driusan commented Dec 19, 2017

@ZainVirani is working on splitting this up, but I'm adding the Block tag and leaving this open so we don't forget about it.

@xlecours
Copy link
Contributor

This work is referenced in Redmine ticket #13780

@xlecours xlecours closed this Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants