This question was originally posted on Stack Overflow.
You can see the before and after there, but here's the after that I'd like to bring for review here.
The goal of this script is to polyfill any browsers I'm supporting that don't natively have position: sticky. Example: I'm thinking about detecting IE11 and running this script only there.
My questions then are:
- Does structuring this code as a class makes sense here?
- Is there anything obvious about my class that's wrong?
- How should I export this as an ES6 module?
- Any other advice?
Code structure is the one big thing I struggle with with JS, and so I'd greatly appreciate any feedback you have.
class makeSticky {
constructor(el) {
this.element = el;
this.fixedClass = 'is-fixed';
this.parent = this.element.parentNode;
this.position = this.element.offsetTop;
this.parentBottom = this.parent.clientHeight + this.parent.offsetTop;
}
init() {
this.addEvents();
this.onScroll();
}
addEvents() {
window.addEventListener('scroll', this.onScroll.bind(this));
}
onScroll(event) {
if (this.aboveScroll() && this.stillInsideParent()) {
this.setFixed();
} else {
this.setStatic();
}
}
aboveScroll() {
return this.position < window.scrollY;
}
stillInsideParent() {
return this.parentBottom > window.scrollY;
}
onScroll(event) {
if (this.aboveScroll() && this.stillInsideParent()) {
this.setFixed();
} else {
this.setStatic();
}
}
setFixed() {
this.element.classList.add(this.fixedClass);
}
setStatic() {
this.element.classList.remove(this.fixedClass);
}
}
const children = document.querySelectorAll('.child');
children.forEach(child => {
const sitck = new makeSticky(child);
sitck.init();
});.
.wrapper {
max-width: 800px;
margin: 60px auto;
}
.is-flex {
display: flex;
}
.sidebar {
width: 300px;
flex: none;
background: #eee;
}
.child {
color: red;
}
.is-fixed {
position: fixed;
top: 20px;
}
<div id="app">
<div class="wrapper is-flex parent">
<div id="main" class="main">
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
</div>
<div class="sidebar">
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<h2 class="child first">Title one should be sticky</h2>
</div>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper is-flex parent">
<div class="main">
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.</p>
</div>
<div class="sidebar">
<h2 class="child two">Title two should be sticky</h2>
</div>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
<div class="wrapper">
<h2>A full width thing might go down here before another wrapper starts</h2>
</div>
</div>
-
\$\begingroup\$ Welcome to Code Review! Please take a look at our FAQ on How to get the best value out of Code Review - Asking Questions. I think the quality of your question can be improved, which usually leads to better answers. \$\endgroup\$Mast– Mast ♦2018年10月21日 07:58:44 +00:00Commented Oct 21, 2018 at 7:58
1 Answer 1
Review
First up Welcome to code review :)
A quick note that the posted code is broken, it throws an error when the snippet runs, and the object makeSticky
has a duplicated property onScroll
As you are new I will say you don't have to accept the first answer that comes along, give it a bit of time as there is more than one school of thought in programing, and questions that have accepted answers don't attract as many new answers.
On to the review.
Your questions
Does structuring this code as a class makes sense here?
No. You keep no reference to it, thus there is no need for an interface. Only the onScroll
function need access to the element and that can be done with closure.
How should I export this as an ES6 module?
If you plan to use it on more than one page then you can wrap it in a function and export that function. Depending on how you plan to use it you can either pass it a list of elements or let it find the elements. See examples below.
Is there anything obvious about my class that's wrong?
Any other advice?
Read on...
Style points
window
is the default object (the globalthis
) and is not needed. . You don't use it forwindow.document
, so why use itwindow.scrollY
andwindow.addEventListener
If you wish to use it there are some edge case where there is a benefit but you should be consistent.- If you are not using an argument don't declare it.
onScroll(event) {
is just as good asonScroll() {
- Functions containing one line of code called only from one place, adds 3 lines of code for no reason. A general rule of thumb for functions is that A function should reduce the overall code size not increase it.
makeSticky
is not a good name, you generally don't start an object's name with a verb, maybestickyElement
or juststicky
Too much bloat
Don't add what is not needed. You define an object makeSticky
and give it 13 exposed (public) properties. When you instantiate it you immediately drop the reference so that the only code that can access the object, is its self, making all the properties and supporting code redundant..
By the looks you only ever create this object in one place, so there really is no need for it to be named in the global scope. And as the object is not referenced elsewhere it can exist as just a closure over the onScroll
listener.
Keep it simple
When writing you should be constantly aware of what is needed and by what.
- Don't add code that will never be used,
- Don't add code for some possible imagined future need, only what is needed (unless its specifically outlined in the design).
- Try to reduce the number of times you need to use
this
Each this means an exposed property, more support code, and greater risk of the object state being mutated.
Examples
As only the onScroll
event listener needs access to the elements all the following examples use closure to maintain the reference. There is no named object for what you called makeSticky
Short and simple
document.querySelectorAll('.child').forEach(child => {
const fixed = 'is-fixed', top = child.offsetTop;
const bot = child.parentNode.clientHeight + child.parentNode.offsetTop;
const onScroll = () => child.classList(top < scrollY && bot > scrollY ? "add" : "remove")(fixed);
addEventListener('scroll', onScroll);
onScroll();
});
or you may prefer a more open style
document.querySelectorAll('.child').forEach(el => {
const fixed = 'is-fixed';
const top = el.offsetTop;
const bot = el.parentNode.clientHeight + el.parentNode.offsetTop;
addEventListener('scroll', onScroll);
(function onScroll() {
if (top < scrollY && bot > scrollY) {
el.classList.add(fixed)
} else {
el.classList.remove(fixed);
}
})();
});
As a module
/* in js file sticky.js */
export default function sticky(selection) {
selection.forEach(el => {
const fixed = 'is-fixed', top = el.offsetTop;
const bot = el.parentNode.clientHeight + el.parentNode.offsetTop;
const onScroll = () => el.classList(top < scrollY && bot > scrollY ? "add": "remove")(fixed);
addEventListener('scroll', onScroll);
onScroll();
});
}
/* In another script */
import sticky from "sticky";
sticky(document.querySelectorAll('.child'));
-
\$\begingroup\$ Wow! Holy cow, what an introduction to this site that I'd never heard about! Thank you for a tremendous answer! Do you happen to know of any books/resources that address some of the issues that are plain to see in my code? A lot of my JS troubles stem from just knowing some code philosophy and the basics of how best to organize things. Thanks again so much!! \$\endgroup\$saltcod– saltcod2018年10月21日 23:30:05 +00:00Commented Oct 21, 2018 at 23:30
-
\$\begingroup\$ Is this a sensible place to ask follow-up questions? Your answer to my "Does structuring this code as a class makes sense here?" was: "No. You keep no reference to it, thus there is no need for an interface. Only the onScroll function need access to the element and that can be done with closure." I don't really know what this means. Also, I think this part of your answer is related: "You define an object makeSticky and give it 13 exposed (public) properties." Is it bad that I make 13 properties public? I assume so, but I'm not really sure why, philosophically. Any tips? \$\endgroup\$saltcod– saltcod2018年10月21日 23:35:32 +00:00Commented Oct 21, 2018 at 23:35
-
\$\begingroup\$ @saltcod Q1 yes the comment are the place to ask follow-ups, but the space is limited. Q2 The last 4 lines of your JS where you create the object
new makeSticky(...
for each".child"
you assign it to theconst stick
(referencing the new object) That reference is held only inside that function meaning you have no way to access each sticky after you create it. This is not bad, as you don't need to, theonScroll
event is the only code that need to. Learning to program means dealing with bugs, more code, more bugs. less code less bugs. (and other benefits) ... continue next comment \$\endgroup\$Blindman67– Blindman672018年10月22日 00:45:18 +00:00Commented Oct 22, 2018 at 0:45 -
\$\begingroup\$ @saltcod One way to reduce code complexity is coding only what is needed, nothing more. 80% your code can be dropped if you use closure to hold the state. Closure is an important part of JS and not an easy thing to understand (Not an easy thing to explain), so investing some time learning what it is and how to use it will go a long way ... continue next comment \$\endgroup\$Blindman67– Blindman672018年10月22日 00:47:17 +00:00Commented Oct 22, 2018 at 0:47
-
\$\begingroup\$ @saltcod I mentioned the 13 properties to make a point, but there is no min or max, good or bad amount, there are only too many when it can be done with less (in this case none). The core point of the review is about code complexity, You had 47 lines, way too much for the task. Anyways learn closure and keep code simple. The full philosophically answer is way beyond the scope of the comments, nor is there just one way... \$\endgroup\$Blindman67– Blindman672018年10月22日 00:49:52 +00:00Commented Oct 22, 2018 at 0:49