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

don't display the sticky nav at the top of the page unless it's appropriate #1266

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Aug 4, 2016

Changes

  • Creates new method stickyNavScrollTopHide on the Navigation prototype in js/navigation.js that removes the show class if the window is scrolled too far up.
  • Separates the cases in Navigation.prototype.stickyNavResizeCallback. The new case groupings are:
    • It's a mobile device: $(window).width() <= 768 , where the sticky nav should initially show at all scroll positions.
    • Configuration options: Largo.sticky_nav_options.sticky_nav_display || ( Largo.sticky_nav_options.main_nav_hide_article && ($('body').hasClass('single') || $('body').hasClass('page')) ), where the sticky nav should not show if the scrollTop is above the main nav.
    • The default case, where the sticky nav should not show.
  • On the scroll event callback Navigation.prototype.stickyNavScrollCallback, prevent the show class from being added to the sticky nav if the page was not previously scrolled. This is needed because Navigation.prototype.stickyNavScrollCallback is called on Navigation.prototype.bindStickyNavEvents is called onNavigation.prototype.bindEventsis called onNavigation.prototype.init` is called when the page is loaded.

Why

After #1262, the sticky nav appears at the top of the page on initial page load, when it should not appear until farther down the page (after scrolling past the main nav). This is part of #1260.

Notes

This PR adds some in-file docs for changes made in js/navigation.js in this PR, but the file in general needs a lot more documentation. Even something like this would be helpful in the future:

An list of what in Navigation calls what
var Navigation = function() {
  return this.init();
    this.enableMobileDropdowns();
    if ($(window).width() > 768) {
      this.stickyNavTransition();
    }   
    this.bindEvents();
      this.bindStickyNavEvents();
        this.stickyNavResizeCallback();
          this.stickyNavSetOffset();
          this.stickyNavTransitionDone();
        this.stickyNavSetOffset();
    this.responsiveNavigation();

@benlk benlk added type: bug priority: high Either blocks work on a priority-normal task or a solution here informs other work. status: needs review labels Aug 4, 2016
@benlk benlk added this to the 0.5.5 - Story Elements milestone Aug 4, 2016
@aschweigert
Copy link

this seems fine to me

@benlk benlk merged commit 04707b9 into develop Aug 4, 2016
@aschweigert aschweigert deleted the 1260-sticky-header-appears-top-wrongly branch September 21, 2016 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Either blocks work on a priority-normal task or a solution here informs other work. type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants