Responsive solution using SASS

Community feedback

• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted about 4 years ago

  Hi Dean, this looks just like the design on desktop, great! I did see a few things to note:

  • max-width is very useful in keeping things fluid. I think the testimonial boxes could use them instead of keeping it fixed for the whole project. Same for the .ratingblocks

  • Interesting implementation for medium widths. I suggest looking into using auto-fit min-max for grid or flex-wrap: wrap for the testimonials. At 750px for example, there is more than enough space for two of them to be on the same row, instead of having that chunk of white space on the right. Noticeably, there is that mobile background in the middle of the design....

  • ....that gets awkward rather quickly :-) . Especially the bottom. Both should be changed way sooner than 1440px.

  • I think the header + the reviews are ready to be side by side before 1000px

  • The component is centered at 1440px, but since you used px for margins to achieve it, it leans to the left on larger screens. One option is to use flex to center horizontally and vertically, and that would also keep it fluid.

  • For your next project, look into using more semantic html structure rather than using divs quite frequently.

  • Lastly, just wondering why you have 700 lines of code. I'd probably use a simple reset for this single component design instead of normalize. Or I would customize the latter. That and delete files I'm not using at all. I'd ask a real expert though @grace-snow :-)

  Hope this helps!

  PS - I also really like include media. It's setup and go :-)

  2

  ---

  

  Dean Hope•230

  @deanhopesPosted about 4 years ago

  @emestabillo

  • I'll go through and check for the max-width you mentioned later on tonight and give it a shot.

  • Yeah I was trying to figure that out but couldn't, so this is great thank you. I'll definitely implement that!

  • You're right, it's very awkward 😂 I'll definitely change this.

  • Again you're right about this and will give it another go.

  • Brilliant. I fell back using pixels as I wasn't sure how to achieve it (it just kept breaking) so I was a bit lazy there. Thanks for this feedback.

  • Yep, divs everywhere. I've seen the memes and I should slap myself for it haha.

  • I think it's because I used a BEM 7-1 template, I deleted a fair bit out of it but when you say it's 700 lines of code that even surprised me haha. Maybe I should check the next time and go through and give it a bit of a trim.

  This is great feedback, will help me grow. Thanks a lot 👍

  1
• 

  T

  Grace•31,230

  @grace-snowPosted about 4 years ago

  Hey Dean, glancing at your sass, I think you need ampersands before your &:nth-child() selectors if you want them to apply to that child.

  I think your BEM naming and html structure looks pretty good.

  Id maybe swap out the divs for block quotes around the testimonials but that's all

  Just taking a look on my phone but the desktop preview on here looks bang on, so you should be very pleased with this

  2

  ---

  

  Dean Hope•230

  @deanhopesPosted about 4 years ago

  Awesome, thank you Grace 👍

  0