2
\$\begingroup\$

I've got a working solution for an onMouseOver event which rolls over a set of images and replaces them in the element. Due to my lack of knowledge in JS I would like to know if this is a proper solution. It works in Chrome and Firefox.

The solution sets attributes (i.e. isRunning="true", isCanceled="true") , reads them depending on the user action (onMouseOver, onMouseOut) and determines if the action should be continued.

Contraint: The solution must stay in Vanilla JS. No libraries may be used.

Thanks for any suggestion!

var thumbNails = {
  123: [
    "https://image.flaticon.com/icons/svg/188/188234.svg",
    "https://image.flaticon.com/icons/svg/188/188235.svg",
    "https://image.flaticon.com/icons/svg/188/188236.svg",
    "https://image.flaticon.com/icons/svg/188/188237.svg",
    "https://image.flaticon.com/icons/svg/188/188238.svg",
    "https://image.flaticon.com/icons/svg/188/188239.svg",
    "https://image.flaticon.com/icons/svg/188/188240.svg",
    "https://image.flaticon.com/icons/svg/188/188241.svg",
    "https://image.flaticon.com/icons/svg/188/188242.svg"
  ]
};

function rollover(element) {
  var id = element.getAttribute("id");
  if (!isRunning(element)) {
    setRunning(element, "true");
    setImageSrc(element, thumbNails[id][0]);
    start(1, thumbNails[id], element);
  }
}

function start(startIndex, thumbNails, element) {
  if (!isCanceled(element) && startIndex < thumbNails.length) {
    setTimeout(function() {
      if (!isCanceled(element)) {
        setImageSrc(element, thumbNails[startIndex]);
      }
      startIndex++;
      console.log("currentIndex: " + startIndex + " - Running? " + isRunning(element) + " - Cancel? " + isCanceled(element));
      start(startIndex, thumbNails, element);
    }, 700);
  } else {
    setRunning(element, "false");
    setCanceled(element, "false");
  }
}

function cancel(element) {
  console.log("cancel");
  if (isRunning(element)) {
    setCanceled(element, "true");
  }
}


// HELPERS

function isRunning(element) {
  return element.getAttribute("isrunning") === "true";
}

function setRunning(element, state) {
  element.setAttribute("isrunning", state);
}

function isCanceled(element) {
  return element.getAttribute("iscanceled") === "true";
}

function setCanceled(element, state) {
  element.setAttribute("iscanceled", state);
}

function setImageSrc(element, url) {
  element.setAttribute("src", url);
}
<html>
<head>
</head>
<body>
  <img id="123" onmouseover="rollover(this)" onmouseout="cancel(this)" src="https://image.flaticon.com/icons/svg/188/188238.svg" width="100" height="100">
</body>
</html>

\$\endgroup\$
1
\$\begingroup\$

Response to your prompt

Due to my lack of knowledge in JS I would like to know if this is a proper solution.

This code sets attributes on the elements to store properties. While this works, it technically leads to invalid HTML1. Alternatives include using data attributes and setting properties on the variable representing the element. See responses to Custom attributes - Yea or nay? and Associate Data With HTML Element (without jQuery)

Other review feedback

Many developers believe it is good to move all JavaScript code out of the markup. So instead of the onmouseover=... and onmouseout=... attributes, you could register those event handlers in the JavaScript using EventTarget.addEventListener()

document.addEventListener('DOMContentLoaded', function() {
  var image = document.getElementById('123');
  image.addEventListener('mouseover', rollover);
  image.addEventListener('mouseout', cancel);
});

With an approach like this, the this keyword inside the functions rollover and cancel refers to the element, so there isn't a need to accept the element as a parameter. If you did want to pass parameters to those functions, it could be achieved using partially applied-functions with Function.bind():

  //set context of 'this' to the image and pass string literal '123' as the first parameter
  image.addEventListener('mouseover', rollover.bind(image, '123'));

The id attribute can be used instead of .getAttribute('id').

So instead of this line:

var id = element.getAttribute("id");

the same can easily be achieved with:

var id = element.id;

This may make assigning that value to a new variable superfluous.

1https://stackoverflow.com/a/13041243/1575353

\$\endgroup\$
  • \$\begingroup\$ Awesome answer, thank you very much. \$\endgroup\$ – rapstacke Jun 6 at 17:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.