Yet Another Sticky Nav PR

[benlk]

Changes

• Remove stickyNavSetOffset() from js/navigation.js for Remove stickyNavSetOffset(); from js/navigation.js #1661, because it's not needed
• Starts a fix for When set to show sticky nav at top of page, sticky nav doesn't #1660:
  • Merges Navigation.prototype.stickyNavScrollCallbacl and Navigation.prototype.stickyNavScrollTopHide
  • removes CSS related to when the sticky nav should be shown or hidden; that should be toggled with the presence or absence of the show class.
• Adds the uglify task to grunt watch
• Clarifies that unchecking the "show sticky nav" box only affects the appearance of the sticky nav on wider screens.

This is ClickUp task IDs #106nk0 and #106rw4

To do:

• review all height- and top-related settings in this script
• see how much further we can refine and reduce this script
• any other fixes for the sticky nav in this version milestone

testing

See #1662 (comment)

[joshdarby]

@benlk This works as expected. I'm going to dive deeper and see if/what we can reduce/refine in navigation.js.

[joshdarby]

@benlk We can probably combine lines 81-82 into
$(window).on('scroll resize', this.stickyNavScrollCallback.bind(this));

https://github.com/INN/largo/blob/c2ff9278d7994a2a2095627c562fe16b366fb600/js/navigation.js#L81-L82

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

Check the status or cancel PullRequest code review here - or - cancel by adding [!pr] to the title of the pull request.

[pullrequest]

Just few readability improvements and simplification

---

Reviewed with ❤️ by PullRequest

[pullrequest]

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

[benlk]

The suggested changes in #1660 (comment) for the problem of the sticky nav not obeying the thing

• the presence or absence of the sticky nav holder should only be toggled by the presence or absence of the show class
• CSS relating to the presence or absence of the sticky nav holder should be limited to:
  • default state hide
  • &.show: show
  • all other showing/hiding CSS removed
    • main_nav_hide_article
    • sticky_nav_display
• Appropriate PHP to ensure that the template starts with the show class in the HTML upon initial page load when it's set to display

Testing criteria for same:

• when the sticky nav is disabled, the sticky nav markup is not output upon the page
• when the sticky nav is enabled, on all pages where the sticky nav is displayed, the sticky nav starts beneath the site logo header.
• scroll behavior of the sticky nav:
  • Scrolling down the page makes the sticky nav go away scrolling up is required to get the sticky nav to display initially
  • scrolling up the page makes the sticky nav reappear
  • scrolling down the page after scrolling up makes the sticky nav disappear again.
• when the sticky nav is set to "Hide the main navigation on article pages and display only the sticky navigation on article pages" in Theme Options > Navigation:
  • the main nav and header do not appear
  • when loading a page at the top of the page, the sticky nav is initially present
  • when loading a page at a point further down the page, the sticky nav is initially present
  • the sticky nav hides and shows as described above
    • Scrolling down the page makes the sticky nav go away
    • scrolling up the page makes the sticky nav reappear
    • scrolling down the page after scrolling up makes the sticky nav disappear again.

[benlk]

... huh. Looks like the "sticky nav disabled" box doesn't do anything, presently. 🐛

[benlk]

When the sticky nav is set to "Hide the main navigation on article pages and display only the sticky navigation on article pages", and the browser is loaded at the top of the page, scrolling down the page, or down-pause-down, does not cause the sticky nav to disappear. This does not occur when the browser is initially loaded from the middle of the page.

[benlk]

The reason for the sticky nav remaining shown is because of how the nav js assumes that #main=nav exists on the page. In the following code, this.mainNavEl = $('#main-nav')

https://github.com/INN/largo/blob/ce2012511a25af60eda69722e55e6acf3eb5cf78/js/navigation.js#L86-L102

If the element is visible, remove 'show', else make sure it shows.

But there's no check that the element exists.

---

an aside: I cannot wait to replace the sticky nav's positional js and suchlike with a big @supports (display: sticky) CSS rule to pin the main nav to the top of the page, below a fullwidth header.

[benlk]

Gonna have to merge stickyNavScrollTopHide and stickyNavScrollCallback; they're fighting each other for dominance it looks like.

[benlk]

73ef9a4 fixes the problem identified in #1662 (comment), where "Scrolling down the page makes the sticky nav go away" was not happening.

[benlk]

when the sticky nav is disabled, the sticky nav markup is not output upon the page

This is partly because of ce20125 removing the styles that kept it hidden, but other changes in this PR removed other code related to the case where sticky_nav_display is false.

[benlk]

Note for testing: With inspector tools docked to the window as a sidebar, the windowwidth() method here returns not the width of the page, but the width of the page plus sidebar, which means the values are erroneous. Testing is better done with the inspector tools detached or in the footer configuration, and the window resized physically.

[benlk]

(fixing merge conflict)

[joshdarby]

All of the test cases in #1662 (comment) work as expected.