1
\$\begingroup\$

I am trying to continuously get the position of a div on screen and update the new position each time a "click" event listener is clicked. At the moment my code works but feels redundant and could be simplified into a single function to move the div around the screen.

Any suggestions on how to approach this issue?

// Init .innerHTML for circle
let startText = document.querySelector('.circle').innerHTML = "click";
let circle = document.querySelector('.circle');

var x = 0;
let titleContainer = document.querySelector('.circle');
let progressContainer = document.querySelector('.progress-container');
let avatar = document.querySelector('.avatar');
let avatarSize = document.querySelector('.avatar').clientWidth;
let totalLevels = 5;

// Listen for events
circle.addEventListener('click', mainClickMaze);

// Start position of smaller circle on progressContainer
function moveAvatarForward() {
  let progressContainerWidth = progressContainer.clientWidth;
  let sum = Math.round(progressContainerWidth / totalLevels);
  console.log(sum);

  requestAnimationFrame(frame);

  let pos = 0;

  function frame() {
    if (pos != sum) {
      pos++;
      avatar.style.marginLeft = pos + "px";
      requestAnimationFrame(frame);
    }
  }
}

// Move the avatar based on its previous location
function locationOfAvatar() {
  let pos = avatar.style.marginLeft;
  // Remove "px" from pos
  pos = pos.substring(0, pos.length - 2);
  pos = parseInt(pos);

  let progressContainerWidth = progressContainer.clientWidth;
  let sum = Math.round(progressContainerWidth / totalLevels);
  let movePos = pos + sum;
  console.log(movePos);
  requestAnimationFrame(frame);

  function frame() {
    if (pos != movePos) {
      pos++;
      avatar.style.marginLeft = pos + "px";
      requestAnimationFrame(frame);
    }
  }
}


// Avatar gets moved to the end of the continer
function endOfAvatar() {
  let pos = avatar.style.marginLeft;
  pos = pos.substring(0, pos.length - 2);
  pos = parseInt(pos);

 // Get size of small circle in progressContainer so that avatarSize does not exceed the bounds of progressContainer
let avatarSize = document.querySelector('.avatar').clientWidth;

let progressContainerWidth = progressContainer.clientWidth;

let sum = progressContainerWidth - avatarSize;
  console.log(sum);

  requestAnimationFrame(frame);

  function frame() {
    if (pos != sum) {
      pos++;
      avatar.style.marginLeft = pos + "px";
      requestAnimationFrame(frame);
    }
  }
}



let click = 0;

function mainClickMaze(event) {
  titleContainer.innerHTML = ++click;
  console.log('click:', click)
  if (click == 1) {
    progressContainer.style.display = 'flex';
  } else if (click == 2) {
    moveAvatarForward();
  } else if (click == 3) {
    locationOfAvatar();
  } else if (click == 4) {
    locationOfAvatar();
  } else if (click == totalLevels) {
    // Remove event listener 
    circle.removeEventListener('click', mainClickMaze);
    titleContainer.innerHTML = 'Disabled';
    endOfAvatar();
  }
}
html,
body {
  padding: 0;
  margin: 0;
}

.container {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  margin-top: 33.3vh;
  margin-bottom: 33.3vh;
}

.circle {
  display: flex;
  width: 20vw;
  height: 20vw;
  background-color: red;
  border-radius: 50%;
  color: white;
  align-items: center;
  justify-content: center;
  text-align: center;
  font-size: 24px;
  cursor: pointer;
}

.move-container {
  display: flex;
  margin-top: 5vh;
  font-size: 24px;
  display: none;
}

.move-back {
  display: flex;
  align-items: center;
  justify-content: center;
  margin-right: 10vw;
  cursor: pointer;
  width: 10vw;
  padding: 10px;
  background-color: grey;
  border-radius: 25px;
  color: white;
}

.move-forward {
  display: flex;
  align-items: center;
  justify-content: center;
  margin-left: 10vw;
  cursor: pointer;
  width: 10vw;
  padding: 10px;
  background-color: grey;
  border-radius: 25px;
  color: white;
}

.progress-container {
  position: fixed;
  display: none;
  align-items: center;
  width: 90%;
  height: 2px;
  background-color: red;
  bottom: 5vh;
}

.avatar {
  width: 2vw;
  height: 2vw;
  background-color: blue;
  border-radius: 50%;
}
<div class="container">
    <div class="circle"></div>
    <div class="move-container">
      <div class="move-back user-click">Back</div>
      <div class="move-forward user-click">Forward</div>
    </div>
    <div class="progress-container">
      <div class="avatar"></div>
    </div>
</div>

Lastly here is a jsfiddle for easier reference: Update position of div

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

Simplifying the functions into one

Yes the three functions can be combined into one. To start, the pos variable can be simplified to default to 0 when no left style has been applied using logical OR because NaN will be converted to false.

let pos = parseInt(avatar.style.marginLeft, 10) || 0;

Notice that the call to parseInt() contains two parameters:

  • the left style of the avatar element without the px characters removed - this is because parseInt() can strip those off.
  • an integer 10 - see feedback point #3 below.

Then a parameter for determining if the avatar should be moved to the end could be added, and since you are using features like let, a default value can be used:

function moveAvatarForward(moveToEnd = false) {

And that value can determine the value of movePos

let movePos = pos + sum;
if (moveToEnd) {
  movePos = progressContainerWidth - avatarSize;
}

Other feedback

  1. Use const whenever a variable doesn't need to be re-assigned
  2. Minimize DOM lookups

    Take for example the first two variable declarations:

    let startText = document.querySelector('.circle').innerHTML = "click";
    let circle = document.querySelector('.circle');
    

    In this case, there are two DOM lookups for the same element. Instead of doing this, the value for startText could be accessed from circle:

    const circle = document.querySelector('.circle');
    const startText = circle.innerHTML = "click";
    

    The same is true for avatarSize - it can be accessed via avatar. And it doesn't need to be re-declared inside endOfAvatar() - just remove the keyword let.

    Additionally, titleContainer points to the same thing as startText so those can be reduced to one variable.

  3. When using parseInt(): "always specify a radix."1 (i.e. 2nd parameter - typically 10)

  4. Use prefix increment in frame() to increment before using a variable. Instead of the following two lines:

    pos++;  
    avatar.style.marginLeft = pos + "px";
    

    Just use the prefix increment to combine the lines:

    avatar.style.marginLeft = ++pos + "px";
    
  5. Avoid poluting the global scope by wrapping the code in a function that is executed once the DOM is ready (e.g. using document.addEventListener() for the DOMContentLoaded event), or at least an IIFE.

Simplified code

The element move-container and its children were removed because they appear to not be used.

document.addEventListener('DOMContentLoaded', function() {
  // Init .innerHTML for circle
  const circle = document.querySelector('.circle');
  circle.innerHTML = "click";
  const progressContainer = document.querySelector('.progress-container');
  const avatar = document.querySelector('.avatar');
  const avatarSize = avatar.clientWidth;
  const totalLevels = 5;

  // Listen for events
  circle.addEventListener('click', mainClickMaze);

  // Move the avatar based on its previous location
  function moveAvatarForward(moveToEnd = false) {
    let pos = parseInt(avatar.style.marginLeft, 10) || 0;
    const progressContainerWidth = progressContainer.clientWidth;
    const sum = Math.round(progressContainerWidth / totalLevels);
    let movePos = pos + sum;
    if (moveToEnd) {
      movePos = progressContainerWidth - avatar.clientWidth;
    }
    requestAnimationFrame(frame);

    function frame() {
      if (pos != movePos) {
        avatar.style.marginLeft = ++pos + "px";
        requestAnimationFrame(frame);
      }
    }
  }

  function mainClickMaze(event) {
    let click = parseInt(circle.innerHTML, 10) || 0;
    circle.innerHTML = ++click;
    if (click == totalLevels) {
      // Remove event listener 
      circle.removeEventListener('click', mainClickMaze);
      circle.innerHTML = 'Disabled';
    }
    if (click == 1) {
      progressContainer.style.display = 'flex';
    } else {
      moveAvatarForward(click == totalLevels);
    }
  }
});
html,
body {
  padding: 0;
  margin: 0;
}

.container {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  margin-top: 33.3vh;
  margin-bottom: 33.3vh;
}

.circle {
  display: flex;
  width: 20vw;
  height: 20vw;
  background-color: red;
  border-radius: 50%;
  color: white;
  align-items: center;
  justify-content: center;
  text-align: center;
  font-size: 24px;
  cursor: pointer;
}

.progress-container {
  position: fixed;
  display: none;
  align-items: center;
  width: 90%;
  height: 2px;
  background-color: red;
  bottom: 5vh;
}

.avatar {
  width: 2vw;
  height: 2vw;
  background-color: blue;
  border-radius: 50%;
}
<div class="container">
  <div class="circle"></div>
  <div class="progress-container">
    <div class="avatar"></div>
  </div>
</div>

\$\endgroup\$
  • \$\begingroup\$ Thank you for the response and the thoroughness of the explanation! \$\endgroup\$ – Joshua Garcia Aug 17 '18 at 2:07

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.