Skip to content

Fix hamburger menu (fixes issue #11) #13

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

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

Conversation

pnijjar
Copy link
Contributor

@pnijjar pnijjar commented May 21, 2017

As indicated in issue #11 , the hamburger menu does nothing when clicked. It looks as if Bootstrap is supposed to include code that toggles this menu, but other than CSS styling I see nothing else in the project that uses Bootstrap. So I wrote a listener that would listen for clicks to the hamburger menu and toggle the menu.

This fixes #11 . (And this should link the two?)

This may not apply cleanly with #12 because both modify incudes to router.js .This should be relatively easy to resolve, however.

I am also not certain where my listener code should be invoked, so I put it in router.js.

js/router.js Outdated

// Maybe this is the wrong place to call this, but
// the hamburger menu applies everywhere.
hamburgerMenuView = new hamburgerMenuView();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this into views/home/home.js. Router shouldn't handle initializing views directly

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please pass in the el: in the constructor instead of defining it directly in the view itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the constructor to pass in the el . All of the other views hardcode their el variables, though. I did not change those.

I am really struggling with moving the constructor for the hamburgerMenuView. I am pretty sure it does not belong in views/home/home.js by itself, because if you navigate to watcamp.com/#links directly then the hamburger menu is broken again. home.js is not a base view; it is specifically for the front page.

I agree that invoking hamburgerMenuView() in the constructor of router.js seems wrong. I just do not understand what is right. As far as I can tell router.js is the only place where views are initialized.

@pnijjar
Copy link
Contributor Author

pnijjar commented Jun 9, 2017

I have been reading into the issue of where to structure Backbone code some more. I think the Backbone Primer has some clues: https://github.com/jashkenas/backbone/wiki/Backbone%2C-The-Primer#getting-view-support:

View management is by far the least regulated component of Backbone, and yet is ironically among the most disciplined roles in front-end engineering. While Backbone.View provides some very useful low-level utility features, it provides few high-level workflow features. As a result, major Backbone extensions including Marionette and LayoutManager have become popular. Also see ContainerView for a minimalist extension of core Backbone.View features.

Adding this listener properly may require incorporating one of these extensions. Given the structure of the code thus far, that seems like a hassle. My preference would be to continue initializing the listener in router.js , but if that is unacceptable then we can look at refactoring the code.

@spaetzel
Copy link
Owner

spaetzel commented Jun 9, 2017

Backbone is pretty open with how to accomplish things. I use it professionally, so have developed my own style and would like to stick with that.

For the hamburger menu specifically, we shouldn't have to do anything, Bootstrap should handle it itself. I'd like to upgrade to the latest bootstrap and try and fix it from there. We definitely don't need to add a backbone view for the hamburger menu.

@pnijjar
Copy link
Contributor Author

pnijjar commented Jun 10, 2017 via email

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.

Hamburger menu is broken?
2 participants