Refactoring footer-button and removing jQuery #2115
Conversation
sebworks
force-pushed
the
footer_button_refactor
branch
from
7717450
to
3a3b643
Compare
| Back to top <span class="cf-icon cf-icon-arrow-up"></span> | ||
| data-js-hook="behavior_return-to-top" | ||
| href="#"> | ||
| Back to top <span clasbs="cf-icon cf-icon-arrow-up"></span> |
Can you add a Jira task specifically for this file if the tests won't be added in this PR. Thanks. |
|
|
||
| $( '.js-return-to-top' ).click( function( event ) { | ||
| // Disable Google Tag Manager tracking on this link. | ||
| behaviorElement.setAttribute( 'data-gtm_ignore', 'true' ); |
There was a problem hiding this comment.
Can we add this attribute in the markup? Seems like we're crossing some streams here.
There was a problem hiding this comment.
The rationale for keeping it in the JS elsewhere is that (a) GTM lives only in JavaScript and (b) in some cases if the default link behavior is being prevented by a particular script, if that script were to be disabled/removed the links should not continue ignoring GTM. In that case it makes sense to set data-gtm_ignore where event.preventDefault() is set.
There was a problem hiding this comment.
The default behavior of an empty anchor link is still to move the user to the top of the page. If for some reason GTM loaded but this script didn't, we should still ignore the interaction in GTM.
There was a problem hiding this comment.
May mean the link should be injected with JS since it doesn't do anything if this script isn't here.
There was a problem hiding this comment.
The button still functions without JS. I'm still trying to figure out if GTM works without JS.
There was a problem hiding this comment.
The link doesn't need to be injected, the jump instead of scroll fallback is perfectly fine. I'm just not convinced we should be injecting metric tracking manipulation into a script that's simply adjusting the button unless that adjustment were to fundamentally change the behavior, which this doesn't. With or without the scrolling action, we don't want this button tracked in GTM. Writing the attr to the markup separates the concerns and ensures GTM isn't tracking the button clicks if this file were to fail in some way.
There was a problem hiding this comment.
I've moved the GTM code to the markup because you will never want to track the event.
|
should this be merged or closed? |
|
@sebworks if you need a second hand getting this through let me know |
sebworks
force-pushed
the
footer_button_refactor
branch
3 times, most recently
from
427c696
to
6d1c6ac
Compare
|
@jimmynotjim, @anselmbradford please give it look when you get a chance. |
|
👍 Can someone on Platform run the tests? I'm so far back I can't get things running correctly. |
|
Can confirm that browser tests pass for me. 👍 |
| } ); | ||
| } | ||
|
|
||
| /** | ||
| * @param {integer} scrollDuration |
There was a problem hiding this comment.
param is not in implementation.
sebworks
force-pushed
the
footer_button_refactor
branch
from
3436c86
to
cc9aa96
Compare
|
👍 |
Refactoring footer-button and removing jQuery
Changes
footer-button.jsto remove jQuery and use requestAnimationFrame for smooth scrolling to the top of page.Testing
localhost:8000on mobile an click theBack to Topbutton. It should smoothly scroll you back to the top of the page.Review
Todos
Checklist