Custom dropdown selector

Question

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 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?

Answer 1

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 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.prototype

You did exactly this, and that is perfect. Keep that up, it is a great practice.

querySelector versus getElementsByClassName

This 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.