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

Scroll to top button added #54

Merged
merged 6 commits into from
Sep 6, 2015
Merged

Scroll to top button added #54

merged 6 commits into from
Sep 6, 2015

Conversation

bogas04
Copy link
Contributor

@bogas04 bogas04 commented Aug 27, 2015

screen shot 2015-08-27 at 11 07 38 pm
I use onscroll event to show scroll to top button only when it is required.
In my initial tests, the button seems to be placed alright for all resolutions.
CSS scroll-behaviour is used to give smooth scroll effect. I feel it is the cleanest way with best case scenario for backward compatibility.

@fhemberger
Copy link
Contributor

Changed a few things:

  • window.pageYOffset instead of e.pageY for cross browser compatibility
  • Use <button> instead of <a href="#"> (when there's no href, always use a button for semantics).
  • Update button styling
  • Indentation
diff --git a/layouts/css/page-modules/_scrollToTop.styl b/layouts/css/page-modules/_scrollToTop.styl
index 495f1c1..58186d1 100644
--- a/layouts/css/page-modules/_scrollToTop.styl
+++ b/layouts/css/page-modules/_scrollToTop.styl
@@ -7,6 +7,12 @@ html
     position fixed
     bottom 10%
     right 5%
+    cursor pointer
     color $node-gray
-    background-color #D9EBB3
+    background-color #eee
+    border 1px solid #ddd
+    border-radius 2px
+    font inherit
+    font-size 1rem
+

diff --git a/layouts/partials/footer.hbs b/layouts/partials/footer.hbs
index b7a6ba6..f5a41d9 100644
--- a/layouts/partials/footer.hbs
+++ b/layouts/partials/footer.hbs
@@ -14,14 +14,13 @@
     </div>
 </footer>

-<a href="#" id="scrollToTop">Scroll To Top</a>
+<button id="scrollToTop">Scroll To Top</button>
 <script type="text/javascript">
-  $scrollToTop = document.getElementById('scrollToTop');
-  window.onscroll = function(e) {
-    $scrollToTop.style.display = (e.pageY > window.innerHeight) ? 'block' : 'none';
-  };
-  $scrollToTop.onclick = function(e) {
-    window.scrollTo(0, 0);
-    return false;
-  };
+    $scrollToTop = document.getElementById('scrollToTop');
+    window.onscroll = function () {
+        $scrollToTop.style.display = (window.pageYOffset > window.innerHeight) ? 'block' : 'none';
+    };
+    $scrollToTop.onclick = function () {
+        window.scrollTo(0, 0);
+    };
 </script>

However, I'm not completely happy with this solution at the moment, because on smaller/mobile screens the button will be placed over the content.


So maybe someone has an idea how to improve this. (Just show an up arrow, e.g. ▲?)
/cc @nodejs/website

@fhemberger
Copy link
Contributor

Oh, and position: fixed might still be a problem on mobile. Haven't tested that yet.

@bogas04
Copy link
Contributor Author

bogas04 commented Aug 28, 2015

I chose anchor tag for cases when javascript is disabled (href="#" would still lead to top). Now that I think of it, not having javascript will not show the button at all. My bad.

I think we can use a media query to change the text to the HTML entity for smaller resolutions. I am ready to take it up.

@phillipj
Copy link
Member

Have you considered throttling the onscroll handler? As it fires alot when scrolling. Or another trick to prevent touching the DOM when not necessary.

@bogas04
Copy link
Contributor Author

bogas04 commented Aug 28, 2015

@phillipj should I use a debounce function ? Or is there an alternative to it ? Or we should show the button what-so-ever ?

@fhemberger
Copy link
Contributor

By the way: How about having a regular "Back to top" button at the end of the page (above the footer) and enhance it with JS to the current behavior? Would be the best solution IMHO.

@bogas04
Copy link
Contributor Author

bogas04 commented Aug 28, 2015

How does it look now ?

  • window.innerWidth > 400px
    screen shot 2015-08-28 at 6 47 55 pm
  • window.innerWidth <= 400px
    screen shot 2015-08-28 at 6 48 13 pm

Note: I haven't added an event listener for resize of browser window for it seems to be overkill for such a small thing. On a default view, mobile user will get the arrow and desktop user will get "Scroll To Top". Mobile user will be okay with the arrow even for landscape view and desktop user will be okay with Scroll To Top even on reducing width, at least that is what I believe.

@fhemberger
Copy link
Contributor

Way better. 👍

For accessibility (and also for not maintaining strings in the JS part), I'd use something like this:

<a href="#" id="scrollToTop"><span>&uarr;</span> Scroll To Top</a>

(restore the default behavior to jump to the top w/o JavaScript) and

#scrollToTop
    ...
    overflow hidden

    span
        display none

    @media (max-width: 400px)
        text-indent -100rem

        span
            display inline

Untested, but you get the idea.

@bogas04
Copy link
Contributor Author

bogas04 commented Aug 28, 2015

I think the arrow with text for desktop and only arrow for mobile users (600px or less) would be neat. CSS is handling all that now. This is how I achieved it :

<a href="#" id="scrollToTop">&uarr; <span>Scroll To Top</span></a>
@media (max-width: 600px)
        span
            display none

Also I'm calling the onclick handler on page load so as to not display it if not required.

(window.onscroll = function() {
    $scrollToTop.style.display = (window.pageYOffset > window.innerHeight) ? 'block' : 'none';
})();

Screenies :

Desktop screen shot 2015-08-28 at 7 40 00 pm Mobile screen shot 2015-08-28 at 7 40 10 pm

FYI : scroll-behavior is only available on FF36 & Chrome(with flag enabled)

@Fishrock123
Copy link
Contributor

Why add a scroll to top button? Most people's mice wheels, trackpads, touchscreens, etc work quite well, no?

@bogas04
Copy link
Contributor Author

bogas04 commented Aug 28, 2015

@Fishrock123 I felt the need while browsing through /contribute/development & /blog. I feel they're long enough to make you realize the time spent scrolling through it.

@phillipj
Copy link
Member

Now that I think of it, not having javascript will not show the button at all.

You will have to add display:none inline or in a .styl for that to be true, as JS is the only thing toggling the display style atm.

<script type="text/javascript">
var $scrollToTop = document.getElementById('scrollToTop');
(window.onscroll = function() {
$scrollToTop.style.display = (window.pageYOffset > window.innerHeight) ? 'block' : 'none';
Copy link
Member

Choose a reason for hiding this comment

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

In regards to my previous throttling question, the simplest fix for as little style changes as possible I could think of:

var newVal = (window.pageYOffset > window.innerHeight) ? 'block' : 'none';
if (this.currentVal !== newVal) {
    $scrollToTop.style.display = newVal;
    this.currentVal = newVal;
}

@fhemberger
Copy link
Contributor

@nodejs/website Do we want to merge this feature for the go-live?

@therebelrobot
Copy link
Contributor

+1 to this PR. There is a bug currently with it briefly showing on page load, but it's definitely not a blocking issue.

@bogas04
Copy link
Contributor Author

bogas04 commented Sep 3, 2015

Let's merge it, it'll be my first code contribution to the project 😀

@fhemberger fhemberger merged commit 29d6530 into nodejs:master Sep 6, 2015
@fhemberger
Copy link
Contributor

@therebelrobot Fixed the flashing of the button on load
@bogas04 Thank you, merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants