-
Notifications
You must be signed in to change notification settings - Fork 17
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
Convert sticky-element.js to use Module pattern #2283
Conversation
0e999a2
to
9991512
Compare
9991512
to
a22463f
Compare
a22463f
to
7b8bbfa
Compare
7b8bbfa
to
41a083f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, some questions about the tests though.
it('hides the element, when scrolled at the top', function () { | ||
instance.start($element) | ||
instance.checkResize() | ||
instance.checkScroll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do these two functions now need to be called where they weren't previously? I can see this in a few of the tests - is this related to not calling the init
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this and your previous comment are related to the init()
function. What I found was that the tests do not test the actual scrolling behaviour - in essence they just test some basic maths and the ability to add/remove classes on an element. I've tested both with and without the init()
function in the beforeEach()
and the behaviour is exactly the same. Which leads me to believe it has no value being run in the tests.
Probably out of scope of this PR - but should we create some tests that actually do test the physical scrolling behaviour?
What Currently, the sticky-element in Government Frontend does not follow the JavaScript module pattern. Convert the existing code to be consistent with this pattern. Why? The module pattern is the standard approach and this JavaScript component does not follow the convention. A note on the tests The tests for sticky-element do not actually test physical scrolling on the page. Instead they test for presence of specific CSS classes. Because of this it was found necessary to run the checkResize() and checkScroll() methods prior to expecting these classes to have changed. In effect this simulates physically scrolling or resizing the page.
41a083f
to
b81240b
Compare
What
Currently, the sticky-element in Government Frontend does not follow the JavaScript module pattern, for example...
Convert the existing code to be consistent with this pattern.
Why do this?
The module pattern is the standard approach and this JavaScript component does not follow the convention.
Trello
Test URLs: