Responsive web app using CSS Grid, Flexbox and JavaScript

Community feedback

• 

  Amon•2,560

  @A-amonPosted about 3 years ago

  Hello! I gotta say, your code is neat and clean 😉

  Here are some suggestions you might find helpful:

  • The .daily, .weekly, .monthly li elements should be/have an interactive element. 🖱 They can be button or a. In my solution, I used button and gave them the role="tab". 😁 You can check out accessible tabs if you want to use the same approach. 😀
  • I believe the images for each .stats aren't that important to be made known to screen readers. Hence, instead of "Icon play", etc. , you can leave it empty alt="". You can read this. 🙌
  • I noticed most of the lines in your JS are similar. For e.g. populateMonthly(), populateWeekly() and populateDaily(), you can create a function populateStats() for e.g., to do all three. The difference in these functions lies in timeframes.weekly.current where the weekly could be replaced with daily or monthly. This can be passed as argument to the newly created populateStats(). 😉

  function populateStats(timeframe) {
    let datacounter = 0;
    cards.forEach(card => {
      const workHours = card.querySelector('.stats__hrs--num');
      const prevWeekHrs = card.querySelector('.stats__prev__hrs');
      workHours.textContent = data[datacounter].timeframes[timeframe].current;
      prevWeekHrs.textContent = data[datacounter].timeframes[timeframe].previous;
      datacounter++;
    });
  }
  

  The code might or might not work. You'll have to test it out and make changes accordingly. 😂

  • forEach comes with index, so you don't have to manually create a datacounter variable to do it. forEach

  cards.forEach((card, index) => {
    ...
  })
  

  By the way, I finally know what to put into typography.scss, thanks to you! 😀 Awesome work~

  Marked as helpful

  3
• 

  Dmitry•375

  @dmitrymitenkoffPosted about 3 years ago

  Hey Amon, Thanks very much for your detailed feedback. I refactored the JS function as you suggested - thanks for that as I'd spent ages trying to figure that out!

  Cheers:)

  0

  ---

  

  nmorajda•945

  @nmorajdaPosted about 3 years ago

  @dmitrymitenkoff Cool! I would even improve the event handling. Instead of writing practically the same three times.

  Sample solution:

  [daily, weekly, monthly].forEach((elem, index, arr) => {
    elem.addEventListener('click', () => {
      //   remove class for all
      arr.forEach(item => item.classList.remove('selected'));
  
      // add class to event emiter
      elem.classList.add('selected');
    });
  });
  

  but it would probably be even better not to use the id selector.

  Marked as helpful

  1

  

  Amon•2,560

  @A-amonPosted about 3 years ago

  @dmitrymitenkoff Glad to help~ 😀

  1

  

  Amon•2,560

  @A-amonPosted about 3 years ago

  @nmorajda Yep, that's true! 😀

  0

  

  Dmitry•375

  @dmitrymitenkoffPosted about 3 years ago

  @nmorajda Thanks so much for this! I've refactored the listeners and the code logic looks so much neater. Cheers

  0

  

  nmorajda•945

  @nmorajdaPosted about 3 years ago

  @dmitrymitenkoff Super! Instead of checking the condition, you can immediately.

  populateStats(button.id) 
  

  let the computers work for us ;)

  1