Skip to content

Call first reload with $timeout #54

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

Merged

Conversation

aantipov
Copy link
Contributor

@aantipov aantipov commented Mar 4, 2016

Dunno why the directive was initialised with confusing $scope.$watch(datasource.revision, () => reload());
I didn't find use of datasource.revision anywhere else, so to make it clear I decided to replace it with $timeout(() => reload());

@mfeingold
Copy link
Contributor

As explained in the description, the idea is (was) that you can set a property called 'revision' on the datasource, the scroller will $watch it. Changing the value of this property will trigger the scroller reload.

It is no longer in the description, but I wonder if people are still using it

@aantipov
Copy link
Contributor Author

aantipov commented Mar 5, 2016

Ok, got it.
So this is an artefact from the past that we should leave as it is for the consistency with previous versions but remove in next major release, right?
If so, why not to mark that string of code as deprecated and add todo to replace it with other code in next major release?

@mfeingold
Copy link
Contributor

On the second though, let us do it. BTW do we even need $timeout here? Would it work if we just call reload at the very end?

@aantipov aantipov force-pushed the call-first-reload-with-timeout branch from 7ebb0a0 to 18cb06b Compare March 7, 2016 17:03
@aantipov
Copy link
Contributor Author

aantipov commented Mar 7, 2016

Agree. I've removed $timeout wrapper and also removed some revision trails from demos.

mfeingold added a commit that referenced this pull request Mar 7, 2016
@mfeingold mfeingold merged commit 216a09c into angular-ui:master Mar 7, 2016
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