Responsive two page design using Sass, BEM, CSS grid, vanilla JS

Community feedback

• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  Hey Agata, well-done on this project! I understand the issue about the select dropdown, it can be quite tricky! :-) I noticed the class naming. I think you were going for BEM + SASS, so on the scss files, you could take advantage of the & to avoid rewriting the parent selector repeatedly. Also with BEM, classnames such as .countdown-clock__timer__card__number seems verbose and goes against the block__element--modifier convention. I'd also prefer to include actual text errors on the form, but it seems to validate ok. Looks good overall :-)

  2

  ---

  

  Agata Liberska•3,995

  @AgataLiberskaPosted almost 4 years ago

  @emestabillo thank you for your comment! I've tried nesting classes with & before and I personally find it hard to find what im looking for 😅 i do struggle with class names though, could you suggest a better option for the class you mention to give me an idea of how to do it better? Is it that the number is not actually a block? And am I ok to add error messages if they're not in the design? I agree that it would be more user friendly 🙂

  1

  

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  @AgataLiberska Maybe the SASS nesting was deeper than it should be and therefore it was harder to look for specific classes. If that is the case, I suggest not nesting beyond 2 levels. For the example above, you could rename it into something like this

  <div class="countdown-clock__timer">
    <div class="countdown-clock__card">
       <div class="countdown-clock__number"></div> //or simply class="number" because I think it's more related to the card rather than the countdown-clock. This is only my opinion though.
       <div class="countdown-clock__text"></div>  // or class="text"
    </div>
  </div>
  

  Basically, you don't need to include each parent element as you go deeper into the html. They can be their own separate element in the block__element scheme.

  You can also reuse the same markup on both timers with the use of modifiers such as .countdown-clock__card--blue

  With the error messages, you'll have to excuse me because I'm not familiar with the design lol. I didn't know it only requires the error icon. Yes you're free to add messages if you'd like to do so. I agree it'd be good for UX :-)

  1

  

  Agata Liberska•3,995

  @AgataLiberskaPosted almost 4 years ago

  @emestabillo ohh I get it now, that's so much easier to read! Thank you!

  1
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  Hi, Agata Liberska! 👋

  This is one of my favorite challenges and I think you've done a great job on this challenge! 🙌 Everything looks good and responds very nicely! 👍

  I think it would be nice if you added a max-width to the pricing cards in the mobile layout (since they are a bit wide when the layout first changes from tablet to mobile). Also, I think it would be cool if the "Try For Free" buttons in the pricing cards took you to the sign-up page (rather than back to the home page). 😉

  I believe I simply used flexbox for the layout of the pricing cards. You can use the flex-wrap property to wrap the content of the card (so long as the card has a width or height that will allow its contents to be wrapped) for displaying the content differently on different screen sizes, but CSS grid might not be a bad way to layout the card, either.

  Keep coding (and happy coding, too)! 😁

  1

  ---

  

  Agata Liberska•3,995

  @AgataLiberskaPosted almost 4 years ago

  @ApplePieGiraffe thank you! I added the links and max width as you suggested - I think I actually tried to set max width on the cards in tablet layout but decided against it eventually, with the hero section in horizontal it looked a bit off-balance (although those very long horizontal cards don't look great either). But restricting the width on mobile does look better, thank you! Now that you mention flexbox, using it would solve the very-long-card problem, wouldn't it? As the card stretches, all elements would end up in one row and fill that empty space up. I guess I have to use flexbox more, I know the basics but I'm not fully comfortable with it so it just didn't occur to me. Thanks for the suggestion!

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @AgataLiberska

  NP! Glad to help! 😀

  I believe if you want elements of a flex container to be wrapped, you'll have to make sure that the width or height of the flex container doesn't grow to accommodate all of the items inside it by default (otherwise, there will be nothing to wrap). You can easily set an explicit width or height, however, and then add flex-wrap: wrap to create a two-dimensional layout! 😉

  1