Scrolling breadcrumb bar #181
Conversation
|
Hey @jwflory😊, I've refactored the breadcrumb bar such that now it hides while scrolling. The title on the navbar is thereafter replaced by the breadcrumb. For fixing the issue, I have added a script tag in the |
justwheel
added
T: improvement
|
Hey @BhavyaT-135, thanks for your patience all this time. I'll add a review here soon. |
|
No problem @jwflory, take your time! 😊 |
justwheel
left a comment
justwheel
left a comment
There was a problem hiding this comment.
Hey @BhavyaT-135, nice work! I cloned this locally and the change worked great. There is room for smoother transitions and animations, but I think it can be addressed in a later PR. The design of this is ready to go.
However, there is invalid HTML because multiple <nav> elements were used in the DOM when there can only be just one. We should follow HTML best practices and make sure that the <nav> is only included once in the DOM. This helps other accessibility devices or scrappers understand the layout of the site more easily.
I left some other comments, but really this is the one thing blocking this from merging. Once it is cleaned up and retested, this should be ready to merge. Let me know if you have any questions! I appreciate your patience in working on this. 🙌🏻
| {{ $logo:= site.Params.logo }} | ||
| {{ $orgName:= site.Params.brand.parent_org_name }} | ||
| {{ $orgUrl:= site.Params.brand.parent_org_url }} | ||
| <div> |
There was a problem hiding this comment.
Was there a reason to add a <div> tag outside of the the <nav> section? This div wraps around the navigation element which feels awkward.
| </nav> | ||
| <nav id="breadcrumbNav"> |
There was a problem hiding this comment.
This is not valid HTML. There should only be one <nav> tag in the DOM. Instead, keep a single <nav> on the start and end of this file, and use <div> tags to create the styling you need.
| <script> | ||
| var topPosition; | ||
|
|
||
| breadcrumbNav = document.getElementById('breadcrumbNav'); | ||
|
|
||
| navbarHeading = document.getElementById('navbarHeading'); | ||
|
|
||
| navbarBreadcrumb = document.getElementById('navbarBreadcrumb'); | ||
|
|
||
| topPosition = window.pageYOffset; | ||
|
|
||
| window.addEventListener('scroll', function () { | ||
|
|
||
| var scrollPosition = window.pageYOffset || document.documentElement.scrollTop; | ||
|
|
||
| if (scrollPosition > topPosition) { | ||
| breadcrumbNav.style.display = 'none'; | ||
| navbarHeading.style.display = 'none'; | ||
| navbarBreadcrumb.style.display = 'block'; | ||
| } | ||
|
|
||
| </div> | ||
| {{ end }} | ||
| </nav> | ||
| else { | ||
| breadcrumbNav.style.display = 'block'; | ||
| navbarHeading.style.display = 'block'; | ||
| navbarBreadcrumb.style.display = 'none'; | ||
| } | ||
| }); | ||
| </script> |
There was a problem hiding this comment.
This is simple enough, but I wonder if it would help maintainability if this script were loaded in <head> instead of here. This way, we could load all JavaScript bits consistently throughout the site, so there is less confusion about how to embed scripts into the DOM.
This change isn't required to merge, but I'd consider it a bonus if you have time to figure out a way to do this similar to other scripts loaded into the DOM.
|
Hey @jwflory, sorry for the late response. I've been a little busy with my university examinations. Will implement the changes as soon as I get some time. 😊 |
|
Hey @jwflory, I hope this resolves the issue of the multiple |
2 participants
Fixes #164