Flex panels in CSS and JS

Question

I was following Wes Bos JS 30-day challenge, so HTML and CSS are mostly are copy-paste, I'd like feedback on JS (mostly). Thanks.

const panels = document.querySelectorAll('.panel');

panels.forEach(panel => panel.addEventListener('click', () => {
  const isOpen = panel.classList.contains('open');
  panels.forEach(panel => panel.classList.remove('open'));
  if(!isOpen) {
    panel.classList.add('open');
  }
}));

panels.forEach(panel => panel.addEventListener('transitionend', e => {
  if(e.propertyName.includes('flex')) {
    panels.forEach(panel => {
      if(panel.classList.contains('open')) {
        panel.classList.add('open-active');
      } else {
        panel.classList.remove('open-active');
      }
    });
  }
}));

html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

body {
  margin: 0;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

.panels {
  min-height: 100vh;
  overflow: hidden;
  display: flex;
}

.panel {
  background: #6B0F9C;
  box-shadow: inset 0 0 0 5px rgba(255, 255, 255, 0.1);
  color: white;
  text-align: center;
  align-items: center;
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  transition:
    font-size 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11),
    flex 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), background 0.2s;
  font-size: 20px;
  background-size: cover;
  background-position: center;

  flex: 1;
  display: flex;
  justify-content: center;
  flex-direction: column;
}

.panel1 {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel2 {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

.panel3 {
  background-image: url(https://images.unsplash.com/photo-1465188162913-8fb5709d6d57?ixlib=rb-0.3.5&q=80&fm=jpg&crop=faces&cs=tinysrgb&w=1500&h=1500&fit=crop&s=967e8a713a4e395260793fc8c802901d);
}

.panel4 {
  background-image: url(https://source.unsplash.com/ITjiVXcwVng/1500x1500);
}

.panel5 {
  background-image: url(https://source.unsplash.com/3MNzGlQM7qs/1500x1500);
}

/* Flex Children */
.panel>* {
  margin: 0;
  width: 100%;
  transition: transform 0.5s;
  flex: 1 0 auto;
  display: flex;
  align-items: center;
  justify-content: center;
}

.panel *:first-child {
  transform: translateY(-100%);
}

.panel *:last-child {
  transform: translateY(100%);
}

.panel.open-active *:first-child,
.panel.open-active *:last-child {
  transform: translateY(0);
}

.panel p {
  text-transform: uppercase;
  font-family: 'Amatic SC', cursive;
  text-shadow: 0 0 4px rgba(0, 0, 0, 0.72), 0 0 14px rgba(0, 0, 0, 0.45);
  font-size: 2em;
}

.panel p:nth-child(2) {
  font-size: 4em;
}

.panel.open {
  font-size: 40px;
  flex: 5;
}

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Flex Panels 💪</title>
  <link href="https://fonts.googleapis.com/css?family=Amatic+SC" rel="stylesheet" type="text/css">
</head>
<body>

  <div class="panels">
    <div class="panel panel1">
      <p>Hey</p>
      <p>Let's</p>
      <p>Dance</p>
    </div>
    <div class="panel panel2">
      <p>Give</p>
      <p>Take</p>
      <p>Receive</p>
    </div>
    <div class="panel panel3">
      <p>Experience</p>
      <p>It</p>
      <p>Today</p>
    </div>
    <div class="panel panel4">
      <p>Give</p>
      <p>All</p>
      <p>You can</p>
    </div>
    <div class="panel panel5">
      <p>Life</p>
      <p>In</p>
      <p>Motion</p>
    </div>
  </div>

</body>
</html>

CertainPerformance · Accepted Answer · 2020-10-31 15:49:27Z

It looks pretty good to me. I can only see a few things to consider:

More precise propertyName check You have if(e.propertyName.includes('flex')) { because Safari uses flex and others use flex-grow. Are you sure that the flex substring won't be present in any other possible CSS transitions? Even if you're sure, will readers of the code be sure? I'd change to an === test against both possibilities, or at least use startsWith (which is a bit more appropriate than .includes here, since both possibilities start with flex).

You can also move the comment about the transition event name to the JS as well as the CSS.

Concise classList setting When you want to either add a class name, or remove a class name, based on a condition, you can condense an if(...) classList.add(...) else(...) classList.remove into a single classList.toggle with a second argument that indicates whether to add or remove the class. Your

if(panel.classList.contains('open')) {
  panel.classList.add('open-active');
} else {
  panel.classList.remove('open-active');
}

simplifies to

const { classList } = panel;
classList.toggle('open-active', classList.contains('open'));

Browser compatibility Though, some ancient browsers don't support the 2nd argument, so consider what sort of browsers you need to support. If you only want to support reasonably up-to-date browsers, it's just fine. Another thing to keep in mind is that NodeList.prototype.forEach was only introduced a few years ago, around 2016 or 2017 IIRC; like startsWith, it's newer than ES6, so either use a polyfill or use iterators and Babel instead, eg:

for (const panel of panels) {
  // do stuff with panel

(if you want to support IE, you should be using Babel anyway, to transpile your code to ES5 syntax)

Void return? panels.forEach(panel => panel.addEventListener returns the value of calling addEventListener to the caller of forEach. Since forEach doesn't look at what its callbacks return, this doesn't do anything. It's not a real problem, but some might consider the code to make a bit more sense if the forEach callback returned void (no return statement or implicit return at all). (Described in TypeScript's TSLint here)

Clickable panels Since the panels are clickable, maybe change from the default cursor to cursor: pointer to make it more obvious to the user that they're meant to be clicked?

Space between elements in selectors I'd change .panel>* to .panel > * - it makes it a bit easier to read when separate elements are separated by spaces.

Repetitive panels Rather than

<div class="panel panel1">
</div>
<div class="panel panel2">
</div>

.panel1 {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel2 {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

Consider using :nth-child instead, allowing you to remove the extra panel# classes entirely.

.panel:nth-child(1) {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel:nth-child(2) {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

const panels = document.querySelectorAll('.panel');

panels.forEach((panel) => {
  panel.addEventListener('click', () => {
    const isOpen = panel.classList.contains('open');
    panels.forEach(panel => panel.classList.remove('open'));
    if (!isOpen) {
      panel.classList.add('open');
    }
  });
});

panels.forEach((panel) => {
  panel.addEventListener('transitionend', e => {
    /* Safari transitionend event.propertyName === flex */
    /* Chrome + FF transitionend event.propertyName === flex-grow */
    if (e.propertyName === 'flex' || e.propertyName === 'flex-grow') {
      panels.forEach(panel => {
        const { classList } = panel;
        classList.toggle('open-active', classList.contains('open'));
      });
    }
  })
});

html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

body {
  margin: 0;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

.panels {
  min-height: 100vh;
  overflow: hidden;
  display: flex;
}

.panel {
  background: #6B0F9C;
  box-shadow: inset 0 0 0 5px rgba(255, 255, 255, 0.1);
  color: white;
  text-align: center;
  align-items: center;
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  transition:
    font-size 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11),
    flex 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), background 0.2s;
  font-size: 20px;
  background-size: cover;
  background-position: center;

  flex: 1;
  display: flex;
  justify-content: center;
  flex-direction: column;
  
  cursor: pointer;
}

.panel:nth-child(1) {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel:nth-child(2) {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

.panel:nth-child(3) {
  background-image: url(https://images.unsplash.com/photo-1465188162913-8fb5709d6d57?ixlib=rb-0.3.5&q=80&fm=jpg&crop=faces&cs=tinysrgb&w=1500&h=1500&fit=crop&s=967e8a713a4e395260793fc8c802901d);
}

.panel:nth-child(4) {
  background-image: url(https://source.unsplash.com/ITjiVXcwVng/1500x1500);
}

.panel:nth-child(5) {
  background-image: url(https://source.unsplash.com/3MNzGlQM7qs/1500x1500);
}

/* Flex Children */
.panel > * {
  margin: 0;
  width: 100%;
  transition: transform 0.5s;
  flex: 1 0 auto;
  display: flex;
  align-items: center;
  justify-content: center;
}

.panel *:first-child {
  transform: translateY(-100%);
}

.panel *:last-child {
  transform: translateY(100%);
}

.panel.open-active *:first-child,
.panel.open-active *:last-child {
  transform: translateY(0);
}

.panel p {
  text-transform: uppercase;
  font-family: 'Amatic SC', cursive;
  text-shadow: 0 0 4px rgba(0, 0, 0, 0.72), 0 0 14px rgba(0, 0, 0, 0.45);
  font-size: 2em;
}

.panel p:nth-child(2) {
  font-size: 4em;
}

.panel.open {
  font-size: 40px;
  flex: 5;
}

<link href="https://fonts.googleapis.com/css?family=Amatic+SC" rel="stylesheet" type="text/css">
<div class="panels">
  <div class="panel">
    <p>Hey</p>
    <p>Let's</p>
    <p>Dance</p>
  </div>
  <div class="panel">
    <p>Give</p>
    <p>Take</p>
    <p>Receive</p>
  </div>
  <div class="panel">
    <p>Experience</p>
    <p>It</p>
    <p>Today</p>
  </div>
  <div class="panel">
    <p>Give</p>
    <p>All</p>
    <p>You can</p>
  </div>
  <div class="panel">
    <p>Life</p>
    <p>In</p>
    <p>Motion</p>
  </div>
</div>

thanks for review and commets! i updated the code based on your comments, but i have questions. 1. what dies this syntax do: const { classList } = panel;? 2. i didn't quite get returning void. isn't it function's user fault to use value of functioning which doesn't return anything? 3. .panel>* was formatted by vscode. how can i turn adding space? — user3132457, Commented Nov 1, 2020 at 8:29
(1) That's destructuring: stackoverflow.com/questions/54605286 Like const classList = panel.classList but more concise. (2) The problem is that it makes the code a tiny bit harder to understand at a glance. It's not forEach's fault that the callback return isn't used, it's the callback's fault for returning something that isn't used anywhere. Not a big deal, just makes the code a bit clearer without returning something that will not be used. — CertainPerformance, Commented Nov 1, 2020 at 15:06
You have got me on the Void return. The OPs code is JS not TypeScript., so the link did not help. I can not work out what you are saying here. Is it that the OP should add the function block delimiters? => {.p.addEv.. rather than => p.addEv... . All JS functions return a value, How do you not imply a return.? Surely you are not trying to say void the return of addEventListener undefined eg panels.forEach(panel => void panel.addEv... which would do nothing (void undefined) === undefined is true. Or am I missing something? — Blindman67, Commented Nov 1, 2020 at 21:52
@Blindman67 Yes, the idea is to add block delimiters so that nothing is returned, implicitly or explicitly. Consider the general case of someFn(arg => someOtherFn(arg)). Without looking at the implementation details, it's not clear whether someFn does something with the callback return value. Not returning anything by using block delimiters makes it clear that there is no link. Sure, most people probably know that forEach's callback return value is ignored anyway, but it'll be a bit clearer to read without having to know that. — CertainPerformance, Commented Nov 1, 2020 at 22:07
Although TS isn't JS, the rationale for the linting rule still applies in JS: "It’s misleading returning the results of an expression whose type is void" — CertainPerformance, Commented Nov 1, 2020 at 22:07

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2020-11-01 11:28:23Z

Review

I agree with the answer by CertainPerformance: the code does look pretty good. Indentation seems consistent, variable names are appropriate and lines are terminated well. Readability is great.

Inefficient loops

The code in the event handlers loops over all panel elements, yet at most only two elements would have updates to their class list. See the suggestion below about ways to eliminate the loops.

Excess CSS rules

The style font-size: 20px; is not needed under .panel since the same rule is also specified on html, plus it gets overridden by more specific selectors.

Suggestion

Remove loops by using Event delegation

Instead of adding event listeners to each panel element, event listeners can be added to the container element. This would require changing the event handlers to look at the event target and determining if the target matched a panel or child of a panel - can be done with the .closest() method. And a live HTMLCollection of elements with class name open can be fetched once using document.getElementsByClassName('open');. Then if any elements have that class when the click handler is called the class name can be removed.

This would allow adding and removing panels without needing to register the event handlers on them. While it may not make a noticable difference on a small page like this, it is wise to consider places where a loop can be avoided.

In the code snippet below, the loops have been eliminated.

const panelsContainer = document.querySelector('.panels');
const openPanels = document.getElementsByClassName('open');
const openActivePanels = document.getElementsByClassName('open-active');

panelsContainer.addEventListener('click', e => {
  const panel = e.target.closest('.panel');
  if (!panel) {
    return;
  }
  const isOpen = panel.classList.contains('open');
  if (openPanels.length) {
    openPanels[0].classList.remove('open');
  }
  panel.classList.toggle('open', !isOpen);
});
panelsContainer.addEventListener('transitionend', e => {
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  if (e.propertyName === 'flex' || e.propertyName === 'flex-grow') {
    if (openActivePanels.length) {
      openActivePanels[0].classList.toggle('open-active', openActivePanels[0].classList.contains('open'))
    }
    if (openPanels.length) {
      openPanels[0].classList.add('open-active')
    }
  }
})

html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

body {
  margin: 0;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

.panels {
  min-height: 100vh;
  overflow: hidden;
  display: flex;
}

.panel {
  background: #6B0F9C;
  box-shadow: inset 0 0 0 5px rgba(255, 255, 255, 0.1);
  color: white;
  text-align: center;
  align-items: center;
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  transition: font-size 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), flex 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), background 0.2s;
  
  background-size: cover;
  background-position: center;
  flex: 1;
  display: flex;
  justify-content: center;
  flex-direction: column;
}

.panel1 {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel2 {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

.panel3 {
  background-image: url(https://images.unsplash.com/photo-1465188162913-8fb5709d6d57?ixlib=rb-0.3.5&q=80&fm=jpg&crop=faces&cs=tinysrgb&w=1500&h=1500&fit=crop&s=967e8a713a4e395260793fc8c802901d);
}

.panel4 {
  background-image: url(https://source.unsplash.com/ITjiVXcwVng/1500x1500);
}

.panel5 {
  background-image: url(https://source.unsplash.com/3MNzGlQM7qs/1500x1500);
}


/* Flex Children */

.panel>* {
  margin: 0;
  width: 100%;
  transition: transform 0.5s;
  flex: 1 0 auto;
  display: flex;
  align-items: center;
  justify-content: center;
}

.panel *:first-child {
  transform: translateY(-100%);
}

.panel *:last-child {
  transform: translateY(100%);
}

.panel.open-active *:first-child,
.panel.open-active *:last-child {
  transform: translateY(0);
}

.panel p {
  text-transform: uppercase;
  font-family: 'Amatic SC', cursive;
  text-shadow: 0 0 4px rgba(0, 0, 0, 0.72), 0 0 14px rgba(0, 0, 0, 0.45);
  font-size: 2em;
}

.panel p:nth-child(2) {
  font-size: 4em;
}

.panel.open {
  font-size: 40px;
  flex: 5;
}

<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <title>Flex Panels 💪</title>
  <link href="https://fonts.googleapis.com/css?family=Amatic+SC" rel="stylesheet" type="text/css">
</head>

<body>

  <div class="panels">
    <div class="panel panel1">
      <p>Hey</p>
      <p>Let's</p>
      <p>Dance</p>
    </div>
    <div class="panel panel2">
      <p>Give</p>
      <p>Take</p>
      <p>Receive</p>
    </div>
    <div class="panel panel3">
      <p>Experience</p>
      <p>It</p>
      <p>Today</p>
    </div>
    <div class="panel panel4">
      <p>Give</p>
      <p>All</p>
      <p>You can</p>
    </div>
    <div class="panel panel5">
      <p>Life</p>
      <p>In</p>
      <p>Motion</p>
    </div>
  </div>

</body>

</html>

thanks for mentioning event delegation, i heard it's important best practice but had completely forgotten about it. just one question, why should i use .closest() instead of e.target.classList.contains('panel')? — user3132457, Commented Nov 1, 2020 at 8:52
That is a great question! It's due to event bubbling which might start at a text node if the user clicks on text within an element. — Sᴀᴍ Onᴇᴌᴀ, Commented Nov 1, 2020 at 11:42

Stack Exchange Network

Flex panels in CSS and JS

2 Answers 2

Review

Inefficient loops

Excess CSS rules

Suggestion

Remove loops by using Event delegation

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
html
css
ecmascript-6
event-handling
or ask your own question.

Hot Network Questions

Flex panels in CSS and JS

2 Answers 2

Review

Inefficient loops

Excess CSS rules

Suggestion

Remove loops by using Event delegation

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascripthtmlcssecmascript-6event-handling or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
javascript
html
css
ecmascript-6
event-handling
or ask your own question.