3

I have 4 images using the class .light-image

I am trying to change them all using js. The code below only grabs the first item, how can I make it grab all 4?

if (window.matchMedia("(max-width: 768px)").matches) {
  document.querySelector('.light-image').src="/app/themes/piranha/assets/public/images/light-open.svg";
}
Justin Blayney
  • 671
  • 1
  • 6
  • 23
  • 8
    You want `for( const el of document.querySelectorAll('.light-image') ) { el.src = "etc"; }` – Dai May 10 '22 at 19:46
  • Thank you for responding, I tried that and it didnt work, it didnt grab any – Justin Blayney May 10 '22 at 19:46
  • 1
    Have you used your debugger? If not, why not? – Dai May 10 '22 at 19:47
  • 2
    @JustinBlayney If you tried just changing to `document.querySelectorAll('.light-image').src="/app/themes/piranha/assets/public/images/light-open.svg";` then it won't work. `querySelectorAll` returns a node list which you need to iterate over and set each elements src property. [querySelectorAll](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll) – Ryan Wilson May 10 '22 at 19:48
  • querySelector**All** should work. Be wary of the camel-cased method name. – kanuos May 10 '22 at 19:48
  • @Ryan Wilson could you show me how that would look please? – Justin Blayney May 10 '22 at 19:49
  • 2
    @JustinBlayney If you scroll down the link I provided for `querySelectorAll` it shows how to iterate the node list. – Ryan Wilson May 10 '22 at 19:51
  • Also: @Dai _showed_ you how you might iterate over the node list in the very first comment. – Andy May 10 '22 at 19:52

2 Answers2

3

Instead of querySelector use querySelectorAll that returns a node list of all elements matching the selector (not just the first one).

Then you need to iterate it over the node list.


if (window.matchMedia("(max-width: 768px)").matches) {
  let itemList = [...document.querySelectorAll('.light-image')]
  itemList.forEach(el => el.src="/app/themes/piranha/assets/public/images/light-open.svg";)
}

See this post for more information

Andy
  • 61,948
  • 13
  • 68
  • 95
lior bakalo
  • 440
  • 2
  • 9
  • 2
    I think you mean [`querySelectorAll`](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll). – Andy May 10 '22 at 19:54
  • Thanks @Andy. Didn't notice that – lior bakalo May 10 '22 at 19:58
  • Thank you all, Dai I cant mark yours as answer – Justin Blayney May 10 '22 at 19:59
  • @JustinBlayney you can upvote his comment – lior bakalo May 10 '22 at 20:01
  • I don't think we should be encouraging people to use `.forEach`. We should direct people to use `for(of)` instead. – Dai May 10 '22 at 21:05
  • @Dai I wouldn't really say one or the other is better in this context, it's simply a matter of whether you prefer a functional or an imperative approach. – Etheryte May 10 '22 at 21:23
  • @Etheryte `.forEach` is demonstrasbly not "functional" because it creates a closure and invariably causes side-effects. `for(of)` is superior in every way, honestly. Anyway, the other reason to avoid `.forEach` is because it isn't defined on `NodeList` (browsers do implement it for compatibility, [but it isn't in the spec](https://dom.spec.whatwg.org/#nodelist)). – Dai May 10 '22 at 21:25
  • @Dai For all practical intents and purposes, none of those nuances really matter. This is similar to a-la worrying about the performance difference between `for` and `forEach`. For nearly every use case it's completely unimportant and trumped by whatever the convention is in the codebase you're in and what's readable. – Etheryte May 10 '22 at 21:34
  • @Etheryte If readability matters then `for(of)` is definitely the winner :) – Dai May 10 '22 at 21:36
  • @JustinBlayney please mark as answer if it helped. – lior bakalo May 11 '22 at 12:32
  • @Dai I wasn't aware of the difference,. I preferred forEach as it is oneLiner. I would love to read more if you have a link – lior bakalo May 11 '22 at 12:33
  • @liorbakalo You can use `for(of)` as a "one-liner" too y'know. As for links and resources, I'm unsure what you're looking for, really. – Dai May 11 '22 at 12:49
0

querySelector vs querySelectorAll

Your current code works on the assumption that document.querySelector always returns a single (and non-null, non-undefined) HTMLImageElement object, whereupon your code immediately sets the .src property.

However, the querySelectorAll function returns a NodeList<TElement> object, which is an iterable collection of HTML elements instead of a singular nullable object reference.

If you're coming from a jQuery background you might be used to how jQuery's $('selector') function always returns a (wrapped) collection of elements, but it has an API design that allows you to set properties and call member functions on all elements in the collection as though the returned object represented a single element.

...unfortunately that style of API design (does it have a name?) doesn't apply anymore (good riddance...), so you need be familiar with how singular (scalar) object references compare to iterables.

In modern JavaScript, when you have an iterable collection you need to use for(of) to access, edit, and work with each object in the collection, like so:

(I changed your selector to img.light-image to prevent any non-<img/> elements from being inadvertently returned).

if (window.matchMedia("(max-width: 768px)").matches) {
  const images = document.querySelectorAll('img.light-image'); // <-- `images: NodeList<HTMLImageElement>`
  for( const img of images ) { // <-- `img: HTMLImageElement`
    img.src = "/app/themes/piranha/assets/public/images/light-open.svg";
  }
}

In old and obsolete (and especially non-portable) JavaScript, like from the days when we had to use jQuery, it was common to use the .forEach member function to succinctly iterate through a collection - however this is unwise, unsafe, and just passé now; namely because .forEach is not well-defined: for example, NodeList<T>.forEach is not standardized or in any formal specifications, according to the MDN.

A better idea:

Fun-fact: you don't need any JavaScript!

What you're trying to accomplish can be achieved using only CSS:

Remove your script and open your .css file (or inline <style> element) and put this instead:

See here: Is it possible to set the equivalent of a src attribute of an img tag in CSS?

@media screen and (max-width: 768px) {

    img.light-image {
        content: url("/app/themes/piranha/assets/public/images/light-open.svg")
    }

}
Dai
  • 141,631
  • 28
  • 261
  • 374