-
Notifications
You must be signed in to change notification settings - Fork 189
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
Always update (allow for controlled components) #80
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak to the reasons this code was there in the first place, but in order for this change to be made, we'd need to add a test that covered it, to prevent regressions.
Yup - adding that now. Thanks. |
Updated original test to reflect new behavior. Code coverage is complaining because the removed lines were covered which shifts the % down slightly. That can be addressed if folks agree this change actually makes sense. |
Any word on when this can be incorporated? It would help me out! |
This will need a rebase on the command line - the "update branch" button adds a merge commit, unfortunately. |
It looks like you rebased your merged branch on top of your fork's master, instead of on top of this repo's master - I'll rebase it for you now. |
thanks - sorry realized that and then got pulled away. |
Proposed fix for #76.