Css Variables and Css Grid Sliding Menu

Css Variables and Css Grid Sliding Menu
0.0 0

#1

Hi, I just finished this sliding menu.
Do you guys have some toughts or improvement to share?
Is it written in a readable manner?

Thank you, I really need some feedback! :slightly_smiling_face:


#2

Well, it was a little confusing a first, because the word Click was there, but nothing would happen when clicking it. You might want to consider adding the same click event handler to the word Click to make it more accessible.

It did work when I clicked on the three bars at the top. However, I did notice if I clicked the three bar icon enough times, the text in the middle of the screen would eventually say undefined. Why? Because eventually you run out of phrases in the story array and when j is larger than 4, you start to get undefined. I would suggest modifying your code to have one more phrase so the length of story matches the length of colors. Then, I would add code to force the values for i and j to reset back to 1 and 0 (respectively) once the last phrase is shown, so you can continue cycling through the phrases and colors forever.

EDIT: Also would also try to make your code DRY (Do Not Repeat Yourself). The following code should be wrapped in a function and called when needed instead of duplicating it.

  a.classList.toggle("rotateRight");
  b.classList.toggle("trasparent");
  c.classList.toggle("rotateLeft");
  e.classList.toggle("darken");
  menu.classList.toggle("move");

Also, the following if/else code is 99% the same. See if you can modify your code to get rid of the 99% duplication.

    if (colorNum == 6) {
      document.documentElement.style.setProperty("--color1", colors[colorNum]);
      document.documentElement.style.setProperty(
        "--color2",
        colors[colorNum - 1]
      );
    } else {
      document.documentElement.style.setProperty("--color1", colors[colorNum]);
      document.documentElement.style.setProperty(
        "--color2",
        colors[colorNum + 1]
      );
    }

Finally, the f.addEventListener(‘click’, function() {…}) should not be nested inside the first click handler. There is no need to keep creating additional click events for f. Move this to the end of your code and it only gets implemented one time like the first click event.


#3

Thank you very much @randelldawson for your insight!
I just changed my code where you suggested.

You said that it was confusing at first because you didn’t know where to click, so I added an arrow-up icon above the “click” text. Do you think is it enough?

Then I made two functions (toggleAnimation, removeAnimation) to keep my code DRY.

I forced the j counter to start from the beginning at the end of the array so the animation keeps looping.

Now I am trying to figure out how to change the if/else statement to reduce the length of my code.


#4

Congratulations! You made your story/colors cycle now. Very nice!

Unfortunately, this change to the code involving the toggled classes is still not DRY. You only need the one function (toggleAnimation). Get rid of removeAnimation and replace each instance where you called it with toggleAnimation.

The only parts different in the if/else statement are the following:

colors[colorNum - 1]

and

colors[colorNum + 1]

The -1 and 1 are just values that you could assign to another variable and either an if/else or the ternary operator to assign it the correct value


#5

Final thoughts…

In general a variable name should describe the data it contains and a function name should describe what the function does. This makes the code more readable.

If you add ids to each of the elements with class names and use the same class name for the id, then you can actually reference the id in your JavaScript without having to use getElementBy or querySelector. Sometimes this is appropriate and sometimes not, so use it when it makes sense.

Get in the habit of declaring all of your variables. I noticed you used the let keyword on one or two variables, but you should be using either let, const, or var on all of them. It is considered a best practice.

With your app, I was able to use this thinking to reduce your JavaScript code even more. See below where I attempt to make your existing code DRY and declare the variables with names which explain what data they contain. I could not figure out what to rename i, but I am sure you can come up with something creative. You will also notice I added one additional function (changeColor) to reduce the duplicate code in the following:

document.documentElement.style.setProperty('--color1', colors[colorNum]);
document.documentElement.style.setProperty('--color2', colors[colorNum + num]);
const colors = ['#FFDA6A', '#FF6D6C', '#41BEFF', '#C54BFF', '#4BFFA6', '#333', '#FFB5C0'];
const phrases = ['This.', 'Is.', 'The Menu.', 'You Were Waiting For.', 'Made with Love. Gez', 'Share (:'];
let i = 1;
let phrasesIndex = 0;

const rotate = function() {
  row1.classList.toggle("rotateRight");
  row2.classList.toggle("trasparent");
  row3.classList.toggle("rotateLeft");
  container.classList.toggle("darken");
  menu.classList.toggle("move");
}

const changeColor = function(colorVar, color) {
  document.documentElement.style.setProperty(colorVar, color);
}

hamburger.addEventListener("click", function() {
  rotate();
  i++;
  if (i % 2 == 1) {
    const colorNum = Math.floor(Math.random() * 6);
    story.innerHTML = phrases[phrasesIndex];
    if (phrasesIndex > 4) {
      phrasesIndex= -1;
    }
    phrasesIndex++; 
    const direction = colorNum == 6 ? -1 : 1;
    changeColor("--color1", colors[colorNum]);
    changeColor("--color2", colors[colorNum + direction]);
  }
});

riempi.addEventListener("click", rotate);

#6

Thank you!

Wow, you are so helpful!!

You made me realize a lot of things that I completely overlooked!
I forgot to declare all the variables! I am definitely a noob :joy:
The code you have reworked is so much cleaner.

You truly gave me a lot of topics to think about.
Now I will study the code and the concepts you posted in your last answer and will implement in my pen!