0

I want an overlay to show up when I click a search icon.

I managed to get it working using jQuery. But can't seem to get it working with javascript.

The click event does not seem to be registering and I don't know why.

I've checked all the class names so they match in the same in both the HTML and javascript

Here is the jQuery code that works:

import $ from 'jquery';

class Search {
  constructor() {
    this.openButton = $('.js-search-trigger');
    this.closeButton = $('.search-overlay__close');
    this.searchOverlay = $(".search-overlay");
    this.events();
  }

  events() {
    this.openButton.on('click', this.openOverlay.bind(this));
    this.closeButton.on('click', this.closeOverlay.bind(this));
  }

  openOverlay() {
    this.searchOverlay.addClass("search-overlay--active");
  }

  closeOverlay() {
    this.searchOverlay.removeClass("search-overlay--active");
  }
}

export default Search;

Here is the javascript code that does not work:

class Search {
  constructor() {
    this.openButton = document.querySelector('.js-search-trigger');
    this.closeButton = document.querySelector('.search-overlay__close');
    this.searchOverlay = document.querySelector('.search-overlay');
    this.events();
  }

  events() {
    this.openButton.addEventListener('click', function() {
      this.openOverlay.bind(this);
    });

    this.closeButton.addEventListener('click', function() {
      this.closeOverlay.bind(this);
    });
  }

  openOverlay() {
    this.searchOverlay.classList.add('search-overlay--active');
  }

  closeOverlay() {
    this.searchOverlay.classList.remove('search-overlay--active');
  }
}

export default Search;

No errors were shown in the javascript where the overlay was not showing.

Jack Bashford
  • 43,180
  • 11
  • 50
  • 79
Ebrahem Luhar
  • 1
  • `$('selector')` gives back a **collection** of elements. `document.querySelector('selector')` gives back only one element, the first to match. So whatever you're doning, without `forEach`-ing your elements it will never be like the jQuery one. And event tje jQuery one will fail if you have multiple overlay elements. – Roko C. Buljan Aug 03 '19 at 13:35
  • 1
    The `function() { }` wrappers around the `.bind()` results are the cause of your problems. – Pointy Aug 03 '19 at 13:37
  • Work to understand the scope of `this` and closure – Mark Schultheiss Aug 03 '19 at 13:40
  • This is a good reference https://stackoverflow.com/q/111102/125981 question on the subject of the issue. See if you can get the "with this" vs "on this" difference – Mark Schultheiss Aug 03 '19 at 13:47
  • So, how many `search-overlay`s will you have and how many `.js-search-trigger` buttons do you plan to have? – Roko C. Buljan Aug 03 '19 at 13:48
  • Thanks for the resource Mark Schultheiss. I used querySelector as there is only one .search-overlay and one .js-search-trigger. – Ebrahem Luhar Aug 03 '19 at 14:37

3 Answers3

3

You'll probably want to change your event listeners to use the correct this binding:

this.openButton.addEventListener("click", this.openOverlay.bind(this));

Or use an arrow function to go with your approach - but make sure you actually call the resulting function, as in the above approach the function is passed as a reference and is called. If you removed the additional () from the code below, it would be the same as writing a function out in your code normally - it would be defined, but nothing would happen.

this.openButton.addEventListener("click", () => {
  this.openOverlay.bind(this)();
});

jQuery also uses collections of elements rather than single elements, so if you have multiple elements, querySelectorAll and forEach might be in order.

Jack Bashford
  • 43,180
  • 11
  • 50
  • 79
0

If we are speaking of ecmascript-6 (I see the tag), I would recommend to use arrow function to have this inherited from the above scope, and no bind is needed:

    this.openButton.addEventListener('click', () => 
      this.openOverlay()
    );

The problems with your code are that a) the function creates new scope with its own this; b) bound methods are not being invoked.

dhilt
  • 18,707
  • 8
  • 70
  • 85
0

Why Search? You're creating an Overlay. Stick with the plan.
No need to bind anything. Use Event.currentTarget if you want to.
No need to handle .open/.close if all you need is a toggle.

And the below should work (as-is) for multiple Overlays. The overlay content is up to you.

class Overlay {

  constructor() {
    this.toggleButtons = document.querySelectorAll('[data-overlay]');
    if (this.toggleButtons.length) this.events();
  }

  events() {
    this.toggleButtons.forEach(el => el.addEventListener('click', this.toggleOverlay));
  }

  toggleOverlay(ev) {
    const btn = ev.currentTarget;
    const sel = btn.getAttribute('data-overlay');
    const overlay = sel ? document.querySelector(sel) : btn.closest('.overlay');
    overlay.classList.toggle('is-active');
  }

}

new Overlay();
*{margin:0; box-sizing:border-box;} html,body {height:100%; font:14px/1.4 sans-serif;}

.overlay {
  background: rgba(0,0,0,0.8);
  color: #fff;
  position: fixed;
  top: 0;
  left: 0;
  width: 100vw;
  height: 100vh;
  padding: 5vw;
  
  transition: opacity 0.4s, visibility 0.4s;
  visibility: hidden;
  opacity: 0;
}
.overlay.is-active {
  opacity: 1;
  visibility: visible;
}
<button type="button" data-overlay="#search">OPEN #search</button>
<button type="button" data-overlay="#qa">OPEN #qa</button>

<div class="overlay" id="search">
  <button type="button" data-overlay>CLOSE</button>
  <h2>SEARCH</h2>
  <input type="text" placeholder="Search&hellip;">
</div>

<div class="overlay" id="qa">
  <button type="button" data-overlay>CLOSE</button>
  <h2>Q&amp;A</h2>
  <ul><li>Lorem ipsum</li></ul>
</div>

The above is still not perfect, still misses a way to "destroy" events and not re-attach duplicate events to already initialised buttons when trying to target dynamically created ones.

Also, the use of Classes for the above task is absolutely misleading and unnecessary.

Roko C. Buljan
  • 196,159
  • 39
  • 305
  • 313