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

Add in UI-Router events / Fix purgeHotkeys #9

Closed
wants to merge 3 commits into from

Conversation

joshbowdoin
Copy link
Contributor

Ignore first commit.

2nd Commit fixes purgHotkeys

3rd Commit adds ui-router compatibility (partially)

Calls ```purgeHotkeys``` on ui-router's $stateChangeSuccess
purgeHotkeys would previously miss the tail end of the array, because it was using a forward loop (angular.forEach), and the index was decrementing with each delete.
@joshbowdoin joshbowdoin changed the title Add in UI-Router events Add in UI-Router events / Fix purgeHotkeys Apr 15, 2014
This is a start on ui-router compatibility.  ```$stateChangeSuccess``` is not called on initial page load, so any shortcuts defined on the first route would not be registered.  I'm not sure how to get around this.
@joshbowdoin
Copy link
Contributor Author

I'm sorry for the multiple commits in one pull request. I'm new to contributing on GitHub, and am not sure how to edit a pull request once I've made it (can I?), and I didn't realize my second pull request was being added to first one. :(

@chieffancypants
Copy link
Owner

Thanks josh! I'll take a close look at this, as I want to make really sure that this will not break current implementations, and I'll need to understand the ui-router in the event a bug comes up, I'll have to be capable of maintaining it moving forward.

You can totally change the code -- just push the changes to your branch (looks like you called it patch-1). You can force push to overwrite the history, which will allow you to undo / split the PR.

@bpaul
Copy link

bpaul commented Apr 23, 2014

👍 Nice update, without this route hotkeys are pretty broken...

@cesarandreu
Copy link

👍 To this! I'm also using ui-router and need this change.

The reference for the ui-router event can be found here.

@joshbowdoin, what would be missing from this PR? I can take a look and add it in.

@smajl
Copy link

smajl commented May 6, 2014

Any progress on this? I am also willing to help with this feature. Just assign tasks what has to be done.

@chieffancypants
Copy link
Owner

Anyone know of a good ui-router app that I can plug this into for testing?

@chieffancypants
Copy link
Owner

I've pulled @joshbowdoin's code into a new branch to work on this. It's been rebased off master so should be up to date, and I fixed a lot of scope issues, and started adding tests.

That said, It's still pretty broken and would really like to see some example ui-router apps so I can see various ways people structure their apps.

Please dig in and help where you can. Closing this PR now that the code has been merged into the ui-router branch. Please issue future PRs against that branch.

Thanks!

@ilanbiala
Copy link

Is this still being pursued?

@niemyjski
Copy link

@chieffancypants what's the status on this?

@niemyjski
Copy link

Be really nice to have this kind of support and this pull request seemed small.

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.

7 participants