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

use watchers and data instead of computed props to memoize results #4372

Merged

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Oct 6, 2018

Summary

Computed props are re-evaluated down the entire chain, whether or not it's necessary. This can be particularly expensive when the prop drives styles or DOM updates.

before after
before after

Reviewer guidance

make sure responsive behaviors are still working, especially grids

References


Contributor Checklist

  • Contributor has fully tested the PR manually
  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If there are any front-end changes, before/after screenshots are included
  • If this is an important user-facing change, PR or related issue has a 'changelog' label

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@indirectlylit indirectlylit added the TODO: needs review Waiting for review label Oct 6, 2018
@indirectlylit indirectlylit added this to the 0.11.0 milestone Oct 6, 2018
@codecov
Copy link

codecov bot commented Oct 6, 2018

Codecov Report

Merging #4372 into develop will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4372      +/-   ##
===========================================
- Coverage     52.7%   52.69%   -0.01%     
===========================================
  Files          721      721              
  Lines        23189    23191       +2     
  Branches      3158     3158              
===========================================
  Hits         12221    12221              
  Misses       10234    10234              
- Partials       734      736       +2
Impacted Files Coverage Δ
...src/views/onboarding-forms/FacilityNameTextbox.vue 93.75% <ø> (ø) ⬆️
...olibri/core/assets/src/mixins/responsive-window.js 84.9% <75%> (-5.1%) ⬇️
...libri/core/assets/src/mixins/responsive-element.js 85.71% <0%> (-14.29%) ⬇️
...bri/plugins/learn/assets/src/views/ContentPage.vue 22.05% <0%> (+0.31%) ⬆️
...ts/src/views/ManageContentPage/ChannelListItem.vue 97.87% <0%> (+2.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c621b...7f92af2. Read the comment docs.

@jonboiser
Copy link
Contributor

The SetupWizardIndex test is failing because of 'DefaultLanguageForm'. You can stub DefaultLanguageForm in the tests, or figure out what the problem is in DefaultLanguageForm.

Having this autosave attr breaks FacilityPermissionsForm-related tests for some reason
@rtibbles rtibbles changed the base branch from develop to release-v0.11.x October 11, 2018 11:48
@indirectlylit
Copy link
Contributor Author

thanks for addressing those issues @jonboiser

was there anything else that I should be doing to get this PR in?

@indirectlylit
Copy link
Contributor Author

thanks!

@indirectlylit indirectlylit merged commit c306879 into learningequality:release-v0.11.x Oct 15, 2018
@indirectlylit indirectlylit deleted the computed-props branch October 17, 2018 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants