QR Code component: Pug SCSS

Community feedback

• 

  P

  Steven Stroud•11,910

  @StroudyPosted 2 months ago

  Hey, fantastic effort on this! You’re really nailing it. Just a few things I noticed that could make it even better…

  • For future project, You could downloading and host your own fonts using @font-face improves website performance by reducing external requests, provides more control over font usage, ensures consistency across browsers, enhances offline availability, and avoids potential issues if third-party font services become unavailable. Place to get .woff2 fonts

  • This does not matter that much at this stage but something to be mindful of for SEO(Search Engine Optimisation), <meta> description tag missing that helps search engine determine what the page is about, Something like this <meta name="description" content="description goes here" />

  You’re doing fantastic! I hope these tips help you as you continue your coding journey. Stay curious and keep experimenting—every challenge is an opportunity to learn. Have fun, and keep coding with confidence! 🌟

  Marked as helpful

  1

  ---

  

  ortiz-antonio•180

  @ortiz-antonioPosted 2 months ago

  Thanks, @Stroudy! I’ve updated the code with your suggestions

  1

  

  P

  Steven Stroud•11,910

  @StroudyPosted 2 months ago

  Hey @ortiz-antonio, I just had a look, Looks great! I got one more for you now since you are hosting your own font,

  • Using font-display: swap in your @font-face rule improves performance by showing fallback text until the custom font loads, preventing a blank screen (flash of invisible text). The downside is a brief flash when the font switches, but it’s usually better than waiting for text to appear.

  I hope to see more from you soon, 💪

  Marked as helpful

  1

  

  ortiz-antonio•180

  @ortiz-antonioPosted 2 months ago

  @Stroudy Done! And of course, follow me to see my other challenges. Thanks

  1

  

  P

  Steven Stroud•11,910

  @StroudyPosted 2 months ago

  Hey @ortiz-antonio, Let's goooo!

  0
• 

  T

  Grace•31,270

  @grace-snowPosted about 2 months ago

  There's a few things I noticed in this that are a bit unusual...

  • it's appearing very narrow to me. I expect it's the large margin causing that. It would be unusual to place margin on a layout like this anyway I think you meant to use padding and in a small fixed unit.
  • the use of em units throughout is really unusual, and potentially problematic, especially if used on a real larger site. Em compounds. You would never want to use em on a font size property. And even on other properties you'd only ever want to use em when you specifically want that property to scale with the element's font-size. That's really not common you'd want that behaviour. It's more common to use rem, percentage or pixels. Each unit choice depends on what behaviour is desired for that specific property. Like em makes sense for letter spacing because it is directly relative to the font size but it definitely doesn't make sense for font-size.
  • For things like the margin on the layour (although I think that should be padding as already said) it would make sense to use pixels or a min() function with px and viewport units and keep it to a much smaller value. Currently, if I change my text size the component will go extremely narrow because the margin will grow too big (viewing on mobile).
  • similar when it comes to padding on the card itself. Keep that small and controlled as a value. You don't want that to change with a users text size setting and certainly not with a parent components font size.
  • you appear to be using an old version of Josh Comeaus reset, not his modern one.
  • the order of items in the css is unusual. Layout would usually be before the component styles not after.
  • beware how much you're nesting scss. It's not a problem but makes it so hard to read. I used to write scss like that but changed after working on a team with established code standards about writing scss and it changes how I wrote it. I've written a post about this: https://fedmentor.dev/posts/sass-nesting/.
  • when you write alt text you must not include words like "image". It's already an image role. Instead this alt text needs to say what the image is (QR code) and where it goes (to FrontendMentor.io).
  • the footer attribution doesn't need to be display flex at all. If you really want to keep it as a flex element it needs a column direction.
  • there's no reason to use a figure elememt in this. That's just an unnecessary wrapper acting like a div. You only need to use figure for content where you need to add a figcaption.
  • think about the context of where this component would be used in a site. It would never be used to serve the main heading on a page, so you know it shouldn't be using a h1. Use a lower importance heading level like h2 instead.

  Marked as helpful

  0

  ---

  

  ortiz-antonio•180

  @ortiz-antonioPosted about 2 months ago

  @grace-snow Thank you, I've implemented your suggestions. I also learned a lot from your blog fedmentor.

  0

  

  T

  Grace•31,270

  @grace-snowPosted about 2 months ago

  @ortiz-antonio I recommend you move the h1 out of the component. These components are meant to be able to be dropped into a real site so the visually hidden h1 shouldn't be in there.

  Also the alt text on the image should be more detailed. As well as saying what the image is (QR code) it needs to say where it goes (to FrontendMentor.io).

  This is all about thinking about how single components eventually get used on a site. These QR code cards might Al go to different sites, be populated with different content, so the alt text needs to be unique.

  I also notice this is hitting screen edges on all sides for me which means it's not like the original design. You don't need all that complex margin stuff. Just give the body a little padding in px.

  And remember font size must not be in px. Use rem. (There is a variable in.px but I don't think you're using it actually so it can be deleted.

  Marked as helpful

  1

  

  ortiz-antonio•180

  @ortiz-antonioPosted about 2 months ago

  @grace-snow I just uploaded the changes. Thank you for the observation about accessibility; I'll keep it in mind for my future projects. Your insight on thinking in components helped me refactor the SASS structure, and I even created a mixin for the QR Pug template to reuse it as a component.

  Regarding the body padding, yes, I changed it to a smaller unit in px and removed the margin in the QR component. I was using a vh unit, but that didn’t make sense since it’s a component. It’s more appropriate to use padding inside the container that holds the component.

  As for the complex part, I checked your solution and was impressed by how you limited the body text length using max-width with a ch unit. It looks great on very small screens, like a smartwatch, because you didn’t need padding in the body text. I didn’t want to copy your solution exactly, so I used the clamp function based on the viewport. When the QR content is too large to fit the viewport (like on a smartwatch), I remove the padding from the body content. However, I’ll definitely use the technique I learned from you in future projects.

  0
• 

  MikDra1•6,980

  @MikDra1Posted 2 months ago

  Well done, here are some things to review 😊:

  • REM for Units: It's best to use rem for all units instead of px, as this ensures scalability and consistency in spacing and font sizes based on the user's root font size. It helps improve accessibility.

  • CSS Variables: Implement CSS variables (--primary-color, --font-size, etc.) for consistent values across the stylesheet. This will allow for easier theme management and tweaking.

  • Clamp() for Responsiveness: Use the clamp() function for fluid typography and spacing, allowing elements to resize smoothly between a minimum and maximum value based on the viewport size (e.g., font-size: clamp(1rem, 2vw, 1.5rem)).

  • Use max-width/min-width and max-height/min-height: Instead of using fixed width and height, opt for max-width or min-width to allow the elements to resize smoothly on different screen sizes, improving overall responsiveness.

  Hope you found this comment helpful 💗💗💗

  Good job and keep going 😁😊😉

  0

  ---

  

  T

  Grace•31,270

  @grace-snowPosted about 2 months ago

  @MikDra1 note that clamp you've recommended would make a site inaccessible. Font size clamp values must convert to rem they can't be viewport only. That's essential.

  Also, it's very rare you'd ever want to use min-width or max- height. Both of those can cause serious breakage if used on components that contain any text content anywhere within them.

  Marked as helpful

  1