Loopstudios - SCSS. Grid, Flexbox, JS, CSS animated mobile menu

Community feedback

• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  Hi Anna, dropping by to say your project looks awesome! I'm also here for the reunion 😂 Seriously though, well-done. Slick mobile menu. I think the grid on the creations sections can wait to be declared until the 750px breakpoint since the design seem to work without it on smaller screens - but this really is just preference.

  1

  ---

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @emestabillo Hi, Em! Thanks for taking the time to look at my solution!

  You're absolutely right about the grid declaration - I originally had it in the 750px media query, but went to see if I could set that section to grid and use grid-gap between the heading, cards, and link for mobile. Unfortunately, the gap between the heading and cards is different from the one between cards and link. I believe you can only use one value for grid-gap, grid-row-gap, and grid-column-gap, right? I think that's why I just went with margins on the card container. Now that I think of it though, I could use grid-gap set to the smaller value, and then use the top margin on the card container to make up the difference...🤔

  1

  

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  @brasspetals Ah I understand now and I know exactly what what you mean. The last sentence is the same “hack” I’ve been using for my layouts lol. Great work replicating the design to the tee 🙂

  1
• 

  Marta•630

  @martam90Posted almost 4 years ago

  Hi Anna,

  I have seen your solution (and others too) and I must say I really admire your pixel perfect solutions. :) I am going to code this challenge and I think your solution might be very inspiring. :)

  1

  ---

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @martam90 Thank you so much for your kind feedback, and I'm glad my solution can provide some inspiration. 😊 I'm looking forward to seeing yours!

  1
• 

  T

  Grace•31,830

  @grace-snowPosted almost 4 years ago

  This looks brilliant, well done!

  I'll try to look at it properly tomorrow on my computer to give feedback on all the accessibility stuff you ask about, but all works great on my phone in the meantime 👍 👍

  1

  ---

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @grace-snow Thanks so much, Grace! Looking forward to your feedback if you get the chance. 😄

  0

  

  T

  Grace•31,830

  @grace-snowPosted almost 4 years ago

  @brasspetals hello!

  I finally had chance today to take a proper look at this and do have a few accessibility tips for you. But they are all pretty minor as you've done a great job on this challenge overall!

  1. With the logo at the moment, my screenreader announces Link, image, Loopstudios logo which doesn't really say what that link does. Instead it might be better to have either the image as aria-hidden and an aria-label on the link that says something like this, or leave the image as visible to assistive tech but change the alt text to say something like this: Loopstudios - Home or just Loopstudios. No-one needs to know it's a logo really

  2. I find the focus states very hard to follow at the moment. There is only the default focus on links, which way too subtle. I'd suggest adding a highly visible solid outline, and using outline-offset to give it a little space from the element.

  3. In a similar vein, the focus state on those creations cards should be removed. Focus shouldn't trigger things like scales and should behave differently to hovers. It's best practice to make focus states fairly consistent, so whatever outline is used on focused links would be used again on these cards.

  4. The creation cards wording is in h3 elements at the moment, but that doesn't make semtic sense. They can't be headings as they have no content under them. Instead, I would make the card container a list, place each link inside list items and swap your h3s for spans inside those links. (not sure why you've set each card to be display grid either?)

  5. The footer__links element should also be a nav element really, with an aria-label to distinguish it from the nav in the header (something like aria-label="footer menu").

  That's all from me. Like I said, most of these are very minor. Really nice work!

  2

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @grace-snow Grace, thank you so much for taking the time to come back to my solution and give such thorough feedback. I super appreciate it!

  1. I’ll change the alt text on the logo to “Loopstudios - Home” per your suggestion as that’s where it would link to were this a real site.

  2. & 3. I’ve unfortunately fallen into a bad habit of just lumping my focus states in with my hover states. Thank you for pointing this out, and I will definitely be more mindful of them here on out (as well as go back in and change these).

  3. I believe I just automatically made them h3s since they had the same style as the rest of the headings, but you’re right - they’re not really headings. Will change it. They’re also not wrapped in individual anchor tags since the whole card is a link. The cards are set to display grid for easier alignment of the text to the bottom left of the cards. Is this bad practice?

  4. I knew the footer link list should be a nav! Thanks for the the confirmation and also the tip to make the header and footer navs distinguishable with an aria-label - very helpful!

  Maybe they were “minor”, but I’m very grateful you brought them to my attention. The accessibility tips were very much appreciated. Thank you again!

  0
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  Hi, Anna! 👋

  It's great to see you submit your solution to this challenge! 😄 You've done such a nice job! 👏

  Your solution's just about pixel-perfect (as usual) and everything responds very nicely! I really like your idea to make the navigation sticky and the smooth animations/transitions on things (like the mobile navigation links) give your solution that extra edge! 😀

  I definitely know what you mean about sticking to as few media queries as possible (it's a nice way to work, I think), but I usually just go ahead and add another media query if I think it'll suit some element or group of elements much better than the existing media queries. I've also noticed that lots of high-quality sites (like Frontend Mentor itself) have tons of media queries, and I'm guessing that might be because when you're working with components (like in React or something) it's easy to just think about how a certain element responds all by itself and add whatever media queries are needed do the job well (as long as your code isn't like super inefficient, I think). 😉

  I found that using the <figure> and <figcaption> elements for the cards and their titles suited them quite nicely and by combining that with the <picture> element (for responsive images), making the background image scale upon hover was pretty easy. I think the way you've done it with pseudo-elements is fine, though (and uses less code, which is nice), so... cool! 👍

  And yeah... I won't say anything about font-size: 62.5% because... well, I kind of started that whole thing. 🙃

  Hey, glad to know you're becoming more comfortable with CSS grid because I certainly feel like I have to re-learn it every time I need to use it for something, LOL. 😅

  Keep coding and happy coding, too, of course! 😁

  1

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @ApplePieGiraffe

  BTW, I noticed you used <br> tags for breaking the main heading onto multiple lines. I've found that simply adjusting the width of an element to force line breaks (which you can do in this case) or wrapping certain words or groups of words in <span> tags can give you a little more control over the way things are laid out in your CSS since you don't have to hard-code line breaks in your HTML. Just saying! 😉

  And also, there's this super-slick stretching effect that happens to the images of the cards when the layout for the grid in the "Creations" section changes. Was that intentional? It's very cool! 😎

  1

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @ApplePieGiraffe Thanks so much! 😊 I’m rather fond of the little sticky navigation transition on scroll, and hope I’ve done it correctly. It was a very late addition to the project. With the transitions/animations, I was really trying to make everything feel “smooth” - mission accomplished! 😎

  In that case, I might have to add those few extra media queries in at some point, unless someone else has a good argument otherwise. Thanks!

  I really do need to learn more about figure and picture and start implementing them.

  😂 It was a bit of a passionate debate, but I thought it was very informative and both sides had valid points. No worries!

  Since flex’s gap property still isn’t fully supported 😭, it’s so nice to be able to utilize it in grid. That’s the biggest appeal for me right now. That and (correct me if I’m wrong), justify and align remain consistent and don’t flip-flop when you change from row/column.

  Ack, I forgot to remove the br’s! I used them when I was first laying out the project for silly reasons. Good catch! I did this on a previous project, and you gave me the same tip about spans - clearly I didn’t learn my lesson. 😅

  At first I wasn’t sure what you meant by the image effect - while I test in all browsers, I typically use Firefox. It seems to be happening in Chromium-based browsers as well as Safari. I believe the transition property on the ::before is being applied to the background-image src switch. 🤔 Neat! 😄 (Now I can’t stop going back and forth watching it, lol.)

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @brasspetals

  Yeah, the flex-gap property will certainly be a great feature to use when it becomes more supported by browsers. 👍 Also, you probably might have already figured this out, but I was just thinking that you perhaps could have animated the background-size property to scale the background of a card when it is hovered over (without having to add an extra pseudo-element), but I think I tried doing that once and found out that there isn't any way to apply a transition to that, so it wouldn't produce a smooth effect (like you nailed throughout the rest of the page). 🙂

  1

  

  T

  Grace•31,830

  @grace-snowPosted almost 4 years ago

  @ApplePieGiraffe gap is supported by all modern browsers now (flex-gap is deprecated)

  You can use it now (and have a fallback by making use of an @supports feature query if you need to support more browsers)

  2

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  @grace-snow

  Oops! That's right, it's the gap property. Thanks—that's good to know! 👍

  0

  

  Anna Leigh•5,135

  @brasspetalsPosted almost 4 years ago

  @ApplePieGiraffe You're right - unfortunately, trying to animate background-size doesn't result in smooth transitions, so I had to find a workaround. Hopefully this gets updated in the future!

  @grace-snow Thanks for bringing up that gap has more support now! Last time I checked, you couldn't simply check for gap with @supports, but had to combine with another condition as well. This article discusses the issue a bit towards the bottom. Is this no longer the case, because if so that would be excellent.

  0