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

Fixes #125 Add reloadOnSearch property to stateConfig #593

Merged
merged 1 commit into from
Nov 23, 2013
Merged

Fixes #125 Add reloadOnSearch property to stateConfig #593

merged 1 commit into from
Nov 23, 2013

Conversation

thoreinstein
Copy link
Contributor

reloadOnSearch when set to false, allows the user to modify the search portion of $location without reloading or reactivating a state.

@timkindberg
Copy link
Contributor

It says build failed, but that failing test is not the one we wrote and was already failing before we added our stuff in.

@timkindberg
Copy link
Contributor

@nateabele one thing I did want to point out in the code as a potential concern (though all tests are passing so we aren't too worried) is that when we were modifying the url with $location.search() it would trigger a state change because the url changed, and although to did equal from because we were going to the same state, we noticed that locals no longer equaled from.locals and it had us stumped and we never could figure out why those would no longer be equal. So our solution was to split that particular expression into its own group within the if statement.

You can see how our if statement require to to equal from, but then after that its either of the other two expressions, not both. I've spaced the statement out onto separate lines for easier comprehension.

if ( 
   to === from && 
   ( 
       (locals === from.locals && !options.reload) || 
       (to.self.reloadOnSearch === false)
   ) 
)

So in essence reloadOnSearch doesn't necessarily care about the $location.search() value, it could potentially be used for other purposes of traveling to the same state without reloading.

Also I'm wondering if we should have tied this in with options.reload somehow.

@nateabele
Copy link
Contributor

I'd rewrite that as if (shouldTriggerReload(to, from, locals options)) { ..., and then extract the condition out into a function.

Also, the tests are failing because the list of states changed. Honestly, we should probably rewrite that test since it's pretty brittle in its current form, but just add 'RS' to that list for now in the proper position. Don't forget to squash your commits.

@timkindberg
Copy link
Contributor

@nateabele good idea on the refactor, will be much easier to read that way.

@janders223 you got this? You've already got the repo pulled down. Need to refactor the if and add 'RS' to the list of states in the breaking test.

@thoreinstein
Copy link
Contributor Author

@timkindberg I'll get on it

@thoreinstein
Copy link
Contributor Author

That should pass the build now.

@nateabele
Copy link
Contributor

Looks good. Squash your commits and we'll roll it.

@timkindberg
Copy link
Contributor

Thanks Jim!

Add reload on search proprty to state config.

Signed-off-by: Jim Anders <[email protected]>

Fixes #125

Add reload on search proprty to state config.

Signed-off-by: Jim Anders <[email protected]>

Extract shouldTiggerReload Function
nateabele added a commit that referenced this pull request Nov 23, 2013
Fixes #125 Add reloadOnSearch property to stateConfig
@nateabele nateabele merged commit 4175179 into angular-ui:master Nov 23, 2013
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.

3 participants