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

Display sticky header on all pages #1262

Merged
merged 13 commits into from
Aug 3, 2016
Merged

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Aug 2, 2016

Changes

  • Rename the sticky_nav_display_article option to sticky_nav_display_all
  • The checkbox for sticky_nav_display_all now controls whether or not the sticky nave displays on all pages on the site or on no pages. If it is checked (by default it is), then sticky navs will appear after scrolling past the main nav.
  • The label for sticky_nav_display_all changes from "Enable the sticky navigation for all screen sizes on article pages" to "Enable the sticky navigation for all screen sizes"
  • Some style changes in less/inc/navbar_sticky.less to allow the navbar to display on all page types, while still being controlled by the checkbox

The sticky nav's appearance behavior is unchanged: it only appears when scrolling upwards, or when the screen width is less than 769px. (for that width, see #1261)

The functionality of the main_nav_hide_article "Hide the main navigation on article pages and display only the sticky navigation on article pages" checkbox is unchanged.

Questions

  • should we rename the theme option from sticky_nav_display_article to something else, to reflect that that theme option now controls whether or not the sticky nav displays on all pages?
    • renamed
  • Are we okay with these styles?

screen shot 2016-08-01 at 4 13 52 pm

screen shot 2016-08-01 at 4 15 17 pm

screen shot 2016-08-01 at 4 15 25 pm

## Why

For #1260.

@benlk benlk added priority: normal Must be completed before release of this version of plugin. type: feature request labels Aug 2, 2016
@benlk
Copy link
Collaborator Author

benlk commented Aug 3, 2016

Ben:

  • go ahead and rename that option
  • assign to Adam

Adam:

  • rewrite UI copy

@@ -79,6 +79,7 @@
Navigation.prototype.bindStickyNavEvents = function() {
var self = this;

// This is so that we may

Choose a reason for hiding this comment

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

may what?

@benlk benlk merged commit 4ad16f9 into develop Aug 3, 2016
@benlk
Copy link
Collaborator Author

benlk commented Aug 3, 2016

Failed tests are the usual; merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal Must be completed before release of this version of plugin. type: feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants