shprink / ionic-native-transitions

[Maintenance only] Native transitions (iOS & Android) for Ionic Framework
https://www.npmjs.com/package/ionic-native-transitions
MIT License
572 stars 112 forks source link

"same state transition are not possible" is not API-consistent with ionic's `$state.go` #119

Closed jakub-g closed 8 years ago

jakub-g commented 8 years ago

https://github.com/shprink/ionic-native-transitions/blob/9192ce31215dd20b583bce126d3fff0d39e67ba4/lib/provider.js#L204

The code of stateGo is as follows

function stateGo(state = null, stateParams = {}, transitionOptions = null, stateOptions = {}) {
            if (!state) {
                $log.debug('[native transition] cannot change state without a state...');
                return;
            }
            if ($state.current.name === state){
                $log.debug('[native transition] same state transition are not possible');
                return;
            }
            unregisterToStateChangeStartEvent();
            transition(transitionOptions);
            return $timeout(() => $state.go(state, stateParams, stateOptions));
}

note the part where the transition is abandoned if we go to the same state

            if ($state.current.name === state){
                $log.debug('[native transition] same state transition are not possible');
                return;
            }

Due to this, we found a regression in our app, because we were doing something like this in some place:

$state.go(state, {}, {reload:true});

Right now when we do

$ionicNativeTransitions.stateGo(..., ..., ..., {reload:true})

the $state.go reload is not triggered.

I think for consistency, ionic-native-transitions should call $state.go in such case.

What do you think?

shprink commented 8 years ago

if reload:true is in Ionic's API then we need to add it too. thanks for reporting.

jakub-g commented 8 years ago

Cool :) Thanks for fast reply.

In fact it's not ionic but ui-router API: http://angular-ui.github.io/ui-router/site/#/api/ui.router.state.$state

reload (v0.2.5) - {boolean=false|string|object}, If true will force transition even if no state or params have changed. It will reload the resolves and views of the current state and parent states (...)

jakub-g commented 8 years ago

Hello @shprink , thanks for quickly fixing this!

Although I had a look at this commit and see that you reversed the order of parameters in the public API. I see that it was done to align with README, but it's an unannounced backward incompatible change that can break stuff for many people, so maybe you want to change the README instead?

shprink commented 8 years ago

The version is now 1.0.0 with a breaking change. it is explained in the changelog https://github.com/shprink/ionic-native-transitions/blob/develop/CHANGELOG.md#100

jakub-g commented 8 years ago

Ok I see that you wanted to align the order of params with ionic, fine, thanks!