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

After animation event support on a per view basis #6

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

CSilivestru
Copy link

Just to preface, I'm quite new to Backbone and js in general so feedback is very much appreciated :-).

I added very basic support for event triggering on a per-view basis. Currently, I was only concerned with triggering events on animation complete. My changes end up providing a very similar syntax to Jr.Navigator.navigate. In the Junior view initialization, I would type something similar to:

Jr.Navigator.addEventListener("animationComplete", "viewURL", callback, context);

Edit:
I added support for a AppView router extension in order to dispose of zombie views and execute any close events that are needed. In addition, I added support for backAnimation complete event and now Jr.Navigator "knows" when were navigating back due to a browser back button push and the AppView router extension allows you to deal with the current view and the next view that we're navigating to.

any unbinding of events that may be needed.

I also added support for browser back button support and to know what
view you're hitting back from and wich you're going to. That info gets
sent to the AppView router extension.
method. Made it too complex for no reason. Added view args to the back
trigger event so that event handlers no the view we are currently
navigating to.
@jschementi
Copy link

I think the project owner would appreciate it if the pull-request did not change the indentation spacing from the original 2-space indent. Style preferences aside, it just makes it much harder to see exactly what you've changed, since the diff shows every line as changed.

@CSilivestru
Copy link
Author

Thanks for the feedback -- I'll make those changes and re-submit and push those along soon. Should have checked... thanks.

@CSilivestru
Copy link
Author

I've changed the indentation back to 2 spaces so the modification are much easier to see now.

@justspamjustin
Copy link
Owner

Thanks so much for the contributions and thanks for changing the tab width. First off, I have a couple of concerns with the code.
junior.js

  • Line 46: viewIdentifier is not a defined variable at this point. Did you mean this.viewIdentifier?
  • Line 57: This usage of a try-catch seems a bit strange to me. Is there some way we can accomplish the same behavior with out using try catch? Perhaps an if-else? My reason for this is because typically try catch should be used for handling errors that may be thrown.
  • Line 58: Jr.viewIdentifier had not been previously defined. Also, I'm not sure viewIdentifier should be a property on the Jr object, because this makes it accessible globally.

Do you mind clarifying a few things?

  • What are the view identifiers for?
  • What is the purpose of Jr.AppView?

Again, I really appreciate the contribution and I apologize for being a bit harsh. Thank you.

@CSilivestru
Copy link
Author

That wasn't harsh at all! I really appreciate the questions since it made me take a closer look at a couple things (I was modifying this code while trying to incorporate in a small project). I'll answer your questions in turn :-).

First, your line comments!

  • I renamed some variables to and changed the viewIdentifier declaration to this.viewIdentifier (like you suggested).
  • I also removed the try catch since it wasn't really needed. This was here to deal with a refresh scenario since the back button flag is set to true by default, a refresh would have provided an undefined for Jr.viewIdentifier (now renamed to Jr.currentView). Now I'm checking for undefined since we don't want a back button event trigger to go off on a refresh.
  • This has been renamed to Jr.currentView. I now define it within Jr. I agree with you that I don't like making this globally accessible.... perhaps my explanation for what these variables are for might help figure out another way of implementing it.

The explanations:

  • I'm using a navigator.viewIdentifier and a Jr.currentView to keep track a string that represents the views in question. navigator.viewIdentifier specifies the view that we're navigating to and Jr.currentView represents, obviously, the view that we're on. I did this to be able to easily dictate view specific event triggers. For example, if I had a view that lists locations from foursquare, I could specify a viewIdentifier on that view as "locations" and then hook onto an event called "animationComplete-locations". I'm very open to other ways of tying in view specific animation triggers -- this was just the simplest way I could think of it on a time crunch :-).
  • Jr. AppView is a small wrapper around the backbone router. Backbone doesn't unbind view events or destroy views once you move off of them (leading to memory problems, orphaned views and events triggered multiple times since they get registered each time you create a view to slide it in). All AppView does is clean up the view you're moving away from before loading in the new one.

I hope that answers your questions reasonably well -- again, I'd love to work through this with you to make it a little bit more robust. Thanks a lot for your input!

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