Highlight anchors in the page contents navigation, on scroll #61
Conversation
gemmaleigh
force-pushed
the
add-nav-highlight
branch
from
2cd1a1e
to
66c8073
Compare
gemmaleigh
changed the title
| expect($anchor.hasClass('active')).toBe(true) | ||
| }) | ||
|
|
||
| it('highlights a nav item, when the setActiveItem function is passed a href', function () { |
There was a problem hiding this comment.
Is this test covered by the test above this one?
There was a problem hiding this comment.
Yes, it's a duplicate - I have removed it.
| } | ||
| } | ||
| module.start($element) | ||
| var $anchor = $element.find('.js-page-contents a') |
There was a problem hiding this comment.
It'd be good if we were more explicit about which link has the 'active' class, rather than assert that one of the links is active.
There was a problem hiding this comment.
I've added a check here for the third item, as this has the active class.
| </div>\ | ||
| </div>\ | ||
| </div>\ | ||
| </div>') |
There was a problem hiding this comment.
I'm wondering how much of this fixture is useful. We're faking the positions of the window scroll and elements.
Maybe a more minimal fixture would:
a) make it easier to read what markup is required
b) would be just as valid a test
There was a problem hiding this comment.
I've simplified the fixture to make it easier to read.
| 'use strict' | ||
|
|
||
| var GOVUK = window.GOVUK | ||
| var $ = window.$ |
There was a problem hiding this comment.
What does this do? Was $ out of scope without it?
There was a problem hiding this comment.
I've removed this, it was copied from another example.
| } | ||
| var testHref = '#section-3' | ||
| module.start($element) | ||
| isLinkHighlighted(testHref) |
There was a problem hiding this comment.
We could probably pass the string literal directly rather than assign it to a variable here.
| </div>\ | ||
| </div>\ | ||
| </div>\ | ||
| </div>') |
| module.start($element) | ||
| var $anchors = $element.find('.js-page-contents a') | ||
| expect($anchors.hasClass('active')).toBe(false) | ||
| }) |
There was a problem hiding this comment.
Just a style thing for fun.. It's quite popular to break tests into paragraphs - setup, exercise and verification paragraphs. It just means putting a line break in between the paragraphs.
eg.
it('has no highlighted nav items, when the page loads', function () {
setWindowScroll(0)
module.start($element)
var $anchors = $element.find('.js-page-contents a')
expect($anchors.hasClass('active')).toBe(false)
})
I also think it'd be cool to hide the complexity of stubbing the window and element position things in a helper function. I had a go in the above example.
None of this essential at all.. Just a suggestion for how I would find it more readable.
gemmaleigh
force-pushed
the
add-nav-highlight
branch
2 times, most recently
from
37007e5
to
981905a
Compare
| $window.scroll(self.hasScrolled) | ||
|
|
||
| setInterval(self.checkResize, _interval) | ||
| setInterval(self.checkScroll, _interval) |
There was a problem hiding this comment.
Is there a reason we're polling at intervals rather than binding directly to the scroll events? If it's to throttle/debounce the scroll event, have we ruled out using e.g. https://github.com/cowboy/jquery-throttle-debounce?
Either way, it'd be good to document why we're doing this as it seems a bit odd to me and difficult to follow…
There was a problem hiding this comment.
I was following the example over here sticky-element-container.js, as it works in a similar way. If there's a better way to do this I am happy to amend.
|
|
||
| self.start = function ($el) { | ||
| $window.resize(self.hasResized) | ||
| $window.scroll(self.hasScrolled) |
There was a problem hiding this comment.
This is totally personal preference, but I've moved to using e.g. .on('scroll', …) as I think it makes it a million times clearer that you're setting up a new binding, vs telling the window to resize or scroll itself.
|
|
||
| self.$anchors = $el.find('.js-page-contents a') | ||
| self.totalAnchors = self.$anchors.length | ||
| self.getAnchors(self.totalAnchors) |
There was a problem hiding this comment.
getAnchors is returning anchorIDs, but the result just gets thrown away? I'd suggest either removing the return from getAnchors to make it clear it's manipulating a variable outside of its own scope, or assigning the result to anchorIDs here.
| self.getAnchors(self.totalAnchors) | ||
|
|
||
| self.checkResize() | ||
| self.checkScroll(self.totalAnchors) |
There was a problem hiding this comment.
checkScroll isn't expecting any arguments?
| self.getAnchors = function (totalAnchors) { | ||
| for (var i = 0; i < self.totalAnchors; i++) { | ||
| var anchor = self.$anchors[i] | ||
| var anchorID = $(anchor).attr('href') |
There was a problem hiding this comment.
If $anchors is an array of jQuery objects, do you need to call jQuery again? anchor.attrshould work? If so, worth naming anchor $anchor instead?
| self.updateActiveNavItem = function () { | ||
| var windowVerticalPosition = self.getWindowPositions().scrollTop | ||
|
|
||
| for (var i = 0; i < self.totalAnchors; i++) { |
There was a problem hiding this comment.
Any reason we're opting for this vs jQuery's .each, as we have jQuery loaded anyway?
gemmaleigh
force-pushed
the
add-nav-highlight
branch
2 times, most recently
from
4d20135
to
54b73b8
Compare
|
The commits since my last review all look good. I think my first two points are still outstanding though? |
|
There are a few really picky edge cases:
However I think those really are edge cases and I'd be happy for this to go out as is, pending the two comments above. |
gemmaleigh
force-pushed
the
add-nav-highlight
branch
from
e151a12
to
c64cdd3
Compare
36degrees
force-pushed
the
add-nav-highlight
branch
from
c64cdd3
to
d7899fe
Compare
gemmaleigh
changed the title
|
This can't be merged just yet, the govuk_frontend_toolkit's The bug spotted by @36degrees, means that when the screen is resized, the shim isn't, so the content in the page contents list is wider than it should be (as the shim prevents the grid-column from resizing the content), leading to the main content overlapping the page contents, at widths wider than 768px. |
gemmaleigh
changed the title
|
I'm now checking for the width of the parent element (in this case This can't be added to the front end toolkit as-is (as there's no guarantee a sticky element will exist within a grid column). Will have to have a think about how best to add this to our app. It may well be better to use our own version of stick-at-top-when-scrolling in this case. |
|
The only thing I've thought of is to modify the Javsacript in front end toolkit to look for a data attribute (or extra class, or similar) on the 'stuck thing' and change its behaviour based on the presence of that attribute. But I don't object to just adding a modified version to our app if that makes more sense for now. Either way, it'd be good to see this merged! |
gemmaleigh
force-pushed
the
add-nav-highlight
branch
6 times, most recently
from
02f337f
to
67c189c
Compare
This already exists within the frontend toolkit, use both stick at top when scrolling (to set the page contents to position:fixed) and stop scrolling at footer, to return the page contents to position: static when the top of the footer reaches the bottom of the page contents.
The navigation highlight module uses this hook for the in-page navigation (js-page-contents). Also add a js hook to the page contents area This is required to get the stick-at-top-when-scrolling js to work. Stick at top when scrolling also has a futher optional hook to set the sticky element and the shim to resize to fit the parent (the hook for this is js-sticky-resize).
gemmaleigh
force-pushed
the
add-nav-highlight
branch
from
67c189c
to
53bdd74
Compare
gemmaleigh
changed the title
These are required by GOVUK.stickAtTopWhenScrolling. Add extra padding on to the bottom of the page contents list, so that it doesn’t allow the Is there anything wrong with this page? link to overlap the bottom of the page contents list.
When the page is scrolled, this will highlight an item in the left hand page contents.
- it has no highlighted nav items, when the page loads - it highlights a nav item, when the page is scrolled Mock the values for scroll position and heading position, and also footer position to ensure the last item is highlighted.
The body of a guide is rendered using a shared ‘GovSpeak’ component. In test mode shared components are not fetched from Static. Instead they are rendered as a dummy tag which contains a JSON dump of the locals - the arguments passed to the component. This means that the headings we expect to be present on the page (within the body of the guide) are not present. This which was causing JavaScript errors on the page, which were appearing in unrelated tests that used the Javascript driver to execute. By returning early if we cannot get the offset of a heading we guard against this.
gemmaleigh
force-pushed
the
add-nav-highlight
branch
from
53bdd74
to
cff0bdd
Compare
This is so that when a page contents anchor is clicked on, the title “Page contents” and the title of the section will line up - rather than the title of the section sitting at the very top of the page.
|
I've also adjusted the margins for the govspeak content, so when an item in the left hand navigation is selected, the "Page contents" title and the section heading line up. Before screenshot: After screenshot: |
|
Looks good to me! |
|
It would be great to get this merged! @36degrees could you please deploy it to integration? |
This PR adds a HighlightActiveSectionHeading module.
This will highlight an anchor link in the "Page contents" left hand navigation, when a user has scrolled past the corresponding heading in the main content area.
This PR also uses the GOVUK.StickAtTopWhenScrolling JS, to "stick" the navigation, once the user has scrolled past the left hand page contents and to "unstick" for smaller viewports.
Steps for this PR:
45ebd8b - duplicating duplicate the GOVUK.StickAtTopWhenScrolling() JS
and
a5f8f41 - revert to use govuk/stick-at-top rather than temp/stick-at-top