All comments

• 

  alexanderokeagu• 20

  @alexanderokeagu

  Submitted over 1 year ago

  All review is welcomed,Thank you.

  

  QR-code-component

  2

  50

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Good job on this, alexanderokeagu. I have a few suggestions for ya :)

  • Try placing your container in a main tag after body for more semantic HTML.

  • You skipped h1 entirely and went straight to h2, unfortunately it is not good practice to skip levels of heading, especially the h1 as this tells visual-impaired users what the whole page even is. You can always put a h1 in a landmark like main and set it to be visually hidden so it does not show up in the page but it is still read out by screen-readers.

  • Your alt text "Your image here" does not make much sense in this context, consider changing it to "QR code to Frontendmentor.io" so someone with a screen-reader can better understand what the image is doing to the website.

  • Try to avoid using px units for pretty much anything, unless you are using decorative properties like borders, box shadow etc.. and want something to look more precise, px units do not scale well when it comes to accessibility features in browsers like zooming in and out. I suggest looking into the rem and em units.

  Here are some references if you feel inclined to read them!

  Semantic HTML via MDN

  Heading Elements via MDN

  Visually Hidden elements via Webaim

  Alt Text via MDN

  "Are you using the right CSS units?" via Kevin Powell on Youtube

  Marked as helpful

  0
• 

  Gerald LeCour• 170

  @Gerald-LeCour

  Submitted over 1 year ago

  Can somone tell me why the icon with the suv is missing?

  Thanks!

  

  Responsive 3 Column Preview Card

  1

  30

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, Gerald. Really good work on this one! I only see a few things I could suggest :)

  Try to only have just one <h1> per page. You could also replace all h1 with <h2> instead and make a h1 titled "3 Column Preview Card" visually hidden so it does not appear on your page but will still be called out by screen readers.

  • (For more information on how to set visually hidden elements, you can read this article by Webaim)

  With your <img> elements, I would suggest leaving the alt blank as they do not have any meaningful content due to being decorative, so screen-readers will avoid calling them out. Also because you have put them in a img tag, screen-readers will already specify that it is an image, so no need to put "icon" or "icon of" etc..

  Because the buttons are labeled as "Learn More", these are better fitted into an <a> tag as they will lead the user to somewhere else. Normally <button> tags are for actions, for example on the current page : validating/resetting a form, showing a modal... While <a> tags are for links and navigation between pages or views.

  Lastly, I would suggest you try to avoid using px units for everything, especially font sizes. They are not accessible when it comes to users who may increase the font size via their browser. Try using em or rem instead as they adapt to browser zooming and are more friendly towards accessibility. Px units are usually best suited for when you want something very specific, decorative elements like borders, border radius or box shadows for example.

  • (For more information on using units and which are best for what, Kevin Powell does a great video explaining a few of them)

  Other than those suggestions, you did a great job on this project, well done!

  Marked as helpful

  1

  This comment has received 1 replyShow
• 

  kays• 50

  @kays20

  Submitted over 1 year ago

  

  Order Summary Componant

  2

  30

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hello, kays20! Good work on this one. I can give you a few suggestions regarding your code. :)

  • Try to refrain from using px units for everything, especially for font sizing. Use rem or em instead. Relative units such as rem and em can adapt to accessibility for when when the users change the browser's font size setting. Px units will always stay the same size as what you set it as.

  (You can read this article regarding which units suit your content)

  • Not mandatory but I suggest looking into if you would want to use a CSS reset whenever you start a new project. This can help you once it comes to styling all your fresh HTML, potentially lowering your chance of running into a lovely CSS snag.

  (There is a lot to choose, if you feel unsure which to use, my recommendation that I use at the moment — Josh Comeau - Custom CSS Reset)

  • Regarding this part of your code:

  @media (min-width: 375px) {
  	body {
  		background-image: url('../images/pattern-background-desktop.svg');
  	}
  

  I would suggest looking into the <picture> tag for when you need to provide different versions of the same image. It saves you having to use a media query.

  (For more information on using <picture>, you can read this article by MDN)

  • Lastly, I noticed in line 92 of your CSS stylesheet, you have a margin-top: 20px; that is commented out, did you forget to delete or un-comment it? :)

  PS: I would suggest reading through the accessibility report that has been automatically made for you and implementing their fixes into your future projects, especially the errors regarding alt text for <img> elements.

  I hope you find these suggestions helpful, overall good effort with this one! 😁

  0
• 

  Victor20231• 10

  @Victor20231

  Submitted over 1 year ago

  

  HTML CSS

  1

  30

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, Victor20231! Good effort on this one! Let's talk about a few things in the accessibility report. :)

  PS: There seems to be a broken link to your github code, this may be difficult for others to give you feedback as they don't have easy access to your repository.

  📝HTML:

  • Document should have one main landmark | All page content should be contained by landmarks

  After your body tag, consider adding landmarks such as, <nav>, <main> and <footer> as a parent to the corresponding content you have where it makes sense. This helps keep your HTML organized and makes it accessible for users using screen-readers, so a win-win for everybody.

  (For more information on what is semantic markup, you can read this article by MDN)

  🎨 CSS:

  • Try to avoid using px units for everything. Pixel units do not scale well once you take into account users who may increase the font size via their browser for accessibility, if the website explicitly sets font-sizes in pixels, a heading set at 30px will always be 30px. Which may cause issues for users with visual impairment.

  rem or em units both respectfully scale when needed and would be much preferred when it comes to font sizing, padding/margins and media queries.

  If you want something to be a constant size that does not need to be responsive to changes in other elements or little things such as borders, border radius or box shadows then pixel units are the way to go.

  (For more information on using units and which are best for what, Kevin Powell does a great video explaining a few of them)

  I hope you find this information helpful. Above all, you did well, well done! 😄

  Marked as helpful

  0
• 

  Ritesh Kumar• 90

  @riteshkumarldh

  Submitted over 1 year ago

  please provide feedback about my code

  

  product preview component - simple grid

  1

  30

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, riteshkumarldh! Great work on this one! Here are a few suggestions regarding your code. :)

  📝HTML:

  • In regards to your <img src="./images/icon-cart.svg" alt="cart">, I would suggest leaving any icons/decorative images (eg: pattern backgrounds) that hold no meaningful content with an empty alt attribute, so screen-readers will ignore it and not confuse the users depending on them.

  (For more information regarding <alt>, you can read this article also by MDN)

  🎨 CSS:

  • Try to avoid using px units for everything. Pixel units do not scale well once you take into account users who may increase the font size via their browser for accessibility, if the website explicitly sets font-sizes in pixels, a heading set at 30px will always be 30px. Which may cause issues for users with visual impairment. rem or em units both respectfully scale when needed and would be much preferred when it comes to font sizing, padding/margins and media queries.

  If you want something to be a constant size that does not need to be responsive to changes in other elements or little things such as borders, border radius or box shadows then pixel units are the way to go.

  (For more information on using units and which are best for what, Kevin Powell does a great video explaining a few of them)

  .btn {
      display: inline-block;
      width: 100%;
      background-color: var(--dark-cyan);
      border: none;
      outline: none;
      height: 60px;
      border-radius: 10px;
      font-size: 18px;
      font-weight: 700;
      color: var(--white);
      display: flex;
      align-items: center;
      justify-content: center;
      gap: 10px;
  }
  

  • Regarding to your code above, you have two duplicate display rules, I would suggest removing one of them as the one below the other will just overwrite it, making it redundant.

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  Marked as helpful

  0

  This comment has received 1 replyShow
• 

  Hyckael• 80

  @Hyckael

  Submitted over 1 year ago

  

  qr code

  2

  50

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, Hyckael! Great work on this one! Let's talk about a few things in the accessibility report. :)

  📝HTML:

  • Page should contain a level-one heading

  Every site must have a h1 where it may make sense, (eg: a title) to describe the main content of your page. This provides vital navigational assistance for users that use assistive systems for impairment, helping them easily find the main content they want to get to.

  If sometimes your page may not have something you think deems as a h1, we can use what is called visually-hidden by hiding it from visual users but will still be called out by a screen-reader for visually impaired users.

  (For more information on this, you can read this article by Webaim)

  • Heading levels should only increase by one

  Ensure all your headings <h1> to <h6> are in a logical order. Try to always start from <h1>, followed by <h2> and so on.

  This is good practice for not only general organization of your page but importantly for users of screen reading software as they may want to jump from different headings to quickly determine the content of the page.

  Not doing so may create confusion, as the user navigating this way may be left wondering where the missing heading is!

  (For more information regarding Headings and how to write them, you can read this article also by MDN)

  🎨 CSS:

  • width= 100% on your body is not needed as it is a family of block elements, (such as html, div and p) it will automatically have a width of 100% as it takes the length of the page.

  @media (max-width:562px) {
      .container{
          height: 80%;
      }
      .card{
       width: 60%;
      }
      .qrContainer{
          height: 60%;
      }
   }
  
   @media (max-width:562px) {
      .container{
          height: 70%;
      }
      .card{
       width: 70%;
      }
   }
  

  • I noticed in the following you have two duplicates, consider removing one of them and merging the two of them all in just one @media query to avoid any possible problems later on.

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  0
• 

  Quinten Raymaekers• 20

  @Quinten-14

  Submitted over 1 year ago

  The sizes and padding is not perfect but everything that needs to be there is. Worked on it for around 10 minutes

  

  QR code with vanilla css and html

  1

  40

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, Quinten-14! Great work on this one! Let's talk about a few things in the accessibility report. :)

  PS: There seems to be a broken link to your github code, this may be difficult for others to give you feedback as they don't have easy access to your repository.

  📝HTML:

  • Document should have one main landmark | All page content should be contained by landmarks

  After your body tag, consider adding landmarks such as, <nav>, <main> and <footer> as a parent to the corresponding content you have where it makes sense. This helps keep your HTML organized and makes it accessible for users using screen-readers, so a win-win for everybody.

  (For more information on what is semantic markup, you can read this article by MDN)

  🎨 CSS:

  • Use min-height: 100vh instead of height on your body. Setting the height to 100vh may result in your content being cut off on smaller screens, such as a mobile phone in landscape orientation or possibly a screen-size below 375px width.

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  Marked as helpful

  1

  This comment has received 1 replyShow
• 

  Divyansh Thakur• 10

  @divyanshthakurx

  Submitted over 1 year ago

  

  Product preview card component solution using HTML & CSS Flexbox

  1

  20

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hi, divyanshthakurx! Great work on this one! Let's talk about a few things in the accessibility report. :)

  📝HTML:

  • Document should have one main landmark | All page content should be contained by landmarks

  After your body tag, consider adding landmarks such as, <nav>, <main> and <footer> as a parent to the corresponding content you have where it makes sense. This helps keep your HTML organized and makes it accessible for users using screen-readers, so a win-win for everybody.

  (For more information on what is semantic markup, you can read this article by MDN)

  • Page should contain a level-one heading

  Every site must have a h1 where it may make sense, (eg: a title) to describe the main content of your page. This provides vital navigational assistance for users that use assistive systems for impairment, helping them easily find the main content they want to get to.

  If sometimes your page may not have something you think deems as a h1, we can use what is called visually-hidden by hiding it from visual users but will still be called out by a screen-reader for visually impaired users.

  (For more information on this, you can read this article by Webaim)

  • Images must have alternate text

  Ensure all your <img> elements have short, descriptive alternate text such as, <alt="drawing of a cat"> as it is incredibly useful for accessibility; screen readers will read your alt description out to their users so they know what the image means while exploring your page.

  Alt text is also displayed on the page if the image can't be loaded for some reason: for example, network errors, content blocking or links expiring.

  For other elements such as icons or images that are purely decorative that hold no meaningful content, you can give it an empty <alt> description so screen-readers will ignore it!

  (For more information regarding <alt>, you can read this article also by MDN)

  🎨 CSS:

  • Use min-height: 100vh instead of height on your body. Setting the height to 100vh may result in your content being cut off on smaller screens, such as a mobile phone in landscape orientation or possibly a screen-size below 375px width.

  • I would suggest maybe increasing the width at which your @media break-point is at, 375px for a desktop design is quite low and as such is causing horizontal scrolling for mobile/tablet until it has enough width to fully show all your content.

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  Marked as helpful

  0
• 

  iamis15• 70

  @iamis15

  Submitted over 1 year ago

  Hello. To whoever's reading, if you have the time, please check out this recreation of a design from this website.

  -The part i found hardest was getting the website to be responsive. Almost all options i tried always ended up in an undesirable result

  • Another thing is measurements. Ive always not known how to use them. or when its appropriate to use rem, em, percentage or pixels.

  I am very new to this so any feedback and tips will be greatly appreciated. Thank you

  

  Project Preview

  3

  40

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hello, iamis15. Great work on this one! Let's talk about a few things in the accessibility report. :)

  📝HTML:

  • Document should have one main landmark | All page content should be contained by landmarks

  After your body tag, consider adding landmarks such as, <nav>, <main> and <footer> as a parent to the corresponding content (such as your div with the id=root) where it makes sense. This helps keep your HTML organized and accessible for users using screen-readers, so a win-win for everybody.

  (For more information on what is semantic markup, you can read this article by MDN)

  • Heading levels should only increase by one

  Ensure all your headings <h1> to <h6> are in a logical order. Try to always start from <h1>, followed by <h2> and so on.

  This is good practice for not only general organization of your page but importantly for users of screen reading software as they may want to jump from different headings to quickly determine the content of the page.

  Not doing so may create confusion, as the user navigating this way may be left wondering where the missing heading is!

  (For more information regarding Headings and how to write them, you can read this article also by MDN)

  • Images must have alternate text

  Ensure all your <img> elements have short, descriptive alternate text such as, <alt="drawing of a cat"> as it is incredibly useful for accessibility; screen readers will read your alt description out to their users so they know what the image means.

  Alt text is also displayed on the page if the image can't be loaded for some reason: for example, network errors, content blocking or links expiring.

  For other elements such as icons or images that are purely decorative that hold no meaningful content, you can give it an empty <alt> description so screen-readers will ignore it!

  (For more information regarding <alt>, you can read this article also by MDN)

  PS: Your repository link seems to be coming up as Page Not Found, is your repository set as public? :)

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  0
• 

  dav9go• 150

  @dav9go

  Submitted over 1 year ago

  I wonder if profesional developers always include all the accesibility options available or its not always necessary.

  

  QR code design with vanilla css

  4

  40

  

  Liam Tanfield• 260

  @TanDevv

  Posted over 1 year ago

  Hello, dav9go. Great work on this one! Let's talk about a few things in the accessibility report. :)

  HTML:

  • Document should have one main landmark | All page content should be contained by landmarks

  After your body tag, consider adding landmarks such as, <nav>, <main> and <footer> as a parent to the corresponding content you have where it makes sense. This helps keep your HTML organized and accessible for users using screen-readers, so a win-win for everybody.

  (For more information on what is semantic markup, you can read this article by MDN)

  • Page should contain a level-one heading

  Every site must have a h1 where it may make sense, (Title) to describe the main content of your page. This provides vital navigational assistance for users that use assistive systems for impairment, helping them easily find the main content they want to get to.

  If sometimes your page may not have something you think deems as a h1, we can use what is called visually-hidden by hiding it from visual users but will still be called out by a screen-reader for visually impaired users.

  (For more information on this, you can read this article by Webaim)

  I hope you find this information helpful. Above all, your solution is great, well done! 😄

  Marked as helpful

  1

  This comment has received 1 replyShow