Skip to content

footer.hbs: tweak scrollToTop. - #2465

Merged
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-scrolltotop
Sep 28, 2019
Merged

footer.hbs: tweak scrollToTop.#2465
Trott merged 1 commit into
nodejs:masterfrom
XhmikosR:master-xmr-scrolltotop

Conversation

@XhmikosR

@XhmikosRXhmikosR commented Aug 25, 2019

Copy link
Copy Markdown
Contributor
  • use addEventListener
  • use event.preventDefault()
  • remove unneeded parentheses
  • remove $ from the variable name

@XhmikosR
footer.hbs: tweak scrollToTop.
1c1686c
* use `addEventListener`
* use `event.preventDefault()`
* remove unneeded parentheses
* remove `$` from the variable name
XhmikosR
XhmikosR commented Aug 25, 2019
View reviewed changes
Comment threadlayouts/partials/footer.hbs
var $scrollToTop = document.getElementById('scroll-to-top');
var scrollToTop = document.getElementById('scroll-to-top');

(window.onscroll = function() {

@XhmikosRXhmikosRAug 25, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If I change this to use addEventListener then the link shows on page load. Maybe this can be improved further, so any ideas welcome.

@XhmikosRXhmikosR mentioned this pull request Sep 8, 2019
Merged
lpinca
lpinca approved these changes Sep 26, 2019
View reviewed changes

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mostly a cosmetic change but LGTM anyway.

@XhmikosR

XhmikosR commented Sep 27, 2019

Copy link
Copy Markdown
ContributorAuthor

@lpinca: how about changing onscroll to use addEventListener? I think it fails because onscroll overwrites any events while addEvenetListener adds a new one. But I feel it would be better if we were consistent here.

@lpinca

lpinca commented Sep 27, 2019

Copy link
Copy Markdown
Member

I'm fine with that if it works.

@XhmikosR

XhmikosR commented Sep 27, 2019

Copy link
Copy Markdown
ContributorAuthor

It doesn't work, hence why I'm asking for ideas :P

@Trott

Trott commented Sep 28, 2019

Copy link
Copy Markdown
Member

Since the current version works, will land this for now so it doesn't stall. The additional troubleshooting/discussion can certainly happen subsequently.

@Trott
Trott merged commit 2b8bdaa into nodejs:masterSep 28, 2019
@XhmikosR
XhmikosR deleted the master-xmr-scrolltotop branch September 28, 2019 14:21
@NOUIYNOUIY mentioned this pull request Jun 10, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@XhmikosR@lpinca@Trott