patternjavascriptMinor
Custom dropdown selector
Viewed 0 times
selectorcustomdropdown
Problem
I'm working on improving my knowledge of OOP in JS. I just created this custom dropdown selector. It's working nicely, but I'm not super happy about the
How you would you simplify that constructor? Any ideas?
Filter constructor. I would like to make all that code more readable. It's basically just defining some attributes and setting some event listeners.function Filter(element) {
this.element = element;
this.dropdown = this.element.querySelector('.dropdown');
var link = this.element.querySelector('.link');
var options = this.dropdown.querySelectorAll('a');
var selectOption = function(event) {
event.preventDefault();
link.textContent = event.target.textContent;
this.toggle();
}
for (var i = 0; i < options.length; i++) {
options[i].addEventListener('click', selectOption.bind(this));
}
link.addEventListener('click', this.toggle.bind(this));
};
Filter.prototype.isExpanded = function() {
return !this.dropdown.classList.contains('hidden');
}
Filter.prototype.collapse = function() {
if (this.isExpanded()) {
this.dropdown.classList.add('hidden');
}
}
Filter.prototype.toggle = function() {
if (!this.isExpanded()) {
var filterExpandedEvent = new CustomEvent('filterExpanded', {detail: this});
document.dispatchEvent(filterExpandedEvent);
}
this.dropdown.classList.toggle('hidden');
}How you would you simplify that constructor? Any ideas?
Solution
Public versus private fields
In Java, it is a custom to make all your fields private, unless they must be explicitly accessed by other sources. I believe this custom also applies to JavaScript.
Instead of having the
And, if there is another place that on using it in, why don't you create a
This is up to you.
Defining the functions with
You did exactly this, and that is perfect. Keep that up, it is a great practice.
This is a matter of efficiency versus readability - whatever you chose to have.
It would be more efficient to use
It is slightly more readable having
Indentation
With only 2 spaces for an indent, your code looks fairly cramped. I recommend using tabs or 4 spaces as an indent.
In Java, it is a custom to make all your fields private, unless they must be explicitly accessed by other sources. I believe this custom also applies to JavaScript.
Instead of having the
element be a public (this) field, why don't you make it a private (var) field? There doesn't really seem to be another place you use it.And, if there is another place that on using it in, why don't you create a
getter? This would be a function that returns the field, and it might be named something like getElement.This is up to you.
Defining the functions with
Filter.prototypeYou did exactly this, and that is perfect. Keep that up, it is a great practice.
querySelector versus getElementsByClassNameThis is a matter of efficiency versus readability - whatever you chose to have.
It would be more efficient to use
getElementsByClassName in place of querySelector in the link field, as the getElementsByClassName function doesn't have to do checks on the argument provided(whether or not the argument is a class, an id, a tag name, or a mixture); it knows it's looking for a one class and one class only.It is slightly more readable having
querySelector, as the syntax for it's arguments is the same syntax for CSS; it's a "universal" syntax.Indentation
With only 2 spaces for an indent, your code looks fairly cramped. I recommend using tabs or 4 spaces as an indent.
Context
StackExchange Code Review Q#79469, answer score: 2
Revisions (0)
No revisions yet.