Skip to content
OverviewSolutions
33
Comments
1141
PhoenixDev22
@PhoenixDev22

All comments

  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Omar Díaz Hernández,

    Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

    • You should use the headers in a chronological order. How you order headings dictates how a screen reader will navigate through them. As you go down a level, the number should increase by one, like a numbered list within an outline. You should have used <h1> for class="main-cover__contents-heading" and <h2> for class="footer-side__data__heading"
    • Avoid creating duplicate content (duplicate navigation). You can style the same navigation in mobile and desktop differently using media queries. Practice like this can result in a poor user experience, when a visitor finds substantially the same content repeated within a set of search results.
    • If you wish to draw an horizontal line which is only for decorative purposes , it is not needed to be announced by a screen reader. You should do so using appropriate CSS.
    • Instead of using a generic div to wrap the navigation links , you put your links within an unordered list structure so that a screen reader will read out how many things are in the list to give visually impaired users the most information possible about the contents of the navigation. The same for the footer's links.
    • The toggle element is added outside the nav, it would be better to be placed within the<nav>. As it is, assistive technology user won’t announce the button related to the <nav>. And this is confusing and not good for the user.

    Toggle Element:

    • It’s not recommended to add event listener on non-interactive elements. You can use a <button> with type=”button”.

    1- The button needs to have anaria-labelattribute or an sr-only text that describes the button purpose. For example, you can have: aria-label='Mobile Navigation Trigger' or 'Open Menu.’

    2- Adding aria-expanded to the button, that way the user will be able to know that the button content controls is expanded or collapsed. At first, it has the “false” as a value then you use JavaScript to change the value.

    3- You should use aria-controls attribute on the toggle element, it should reference the id value of the <ul> element.

    • In class="card-contents", you should never use <div> and <span>alone to wrap a meaningful content. Just keep in mind that you should usually use semantic HTML in place of the div tag unless none of them (the semantic tags) really match the content to group together. By adding semantic tags to your document, you provide additional information about the document, which aids in communication.
    • You should use the<nav > landmark to wrap the footer navigation. Then you should add aria-label=”secondary “ or aria-label=”footer” to it. A brief description of the purpose of the navigation, omitting the term "navigation", as the screen reader will read both the role and the contents of the label.
    • Thenavelement in the header could use an aria-label="primary" or aria-label=”main” attribute on it. The reason for this is that, you should add the aria-label for a nav element if you are using the nav more than once on the page.You can read more in MDN
    • The social links wrapping the svgs must have aria-label or sr-only text indicate where the link will take the user. Then you set aria-hidden =”true” and focusable=”false” to the svgs to be ignored by assistive technology .

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Marco Lizardo Del Riego,

    Congratulation on finishing your first challenge.

    Great job on this one! you have already received some helpful feedback which is nice to see , just going to add some suggestions as well if you don't mind:

    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • The cart image in the button is a decorative image. For decorative svgs, you set anaria-hidden=”true” and focusable=”false” to remove that element from the accessibility tree. This can improve the experience for assistive technology users by hiding purely decorative svgs.
    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.

    hopefully this feedback helps.

  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi ahmed,

    Well done! I have some suggestions regarding your solution if you don't mind:

    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.
    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • The cart image in the button is a decorative image. For decorative images, you set an empty alt to it with an aria-hidden=”true” to remove that element from the accessibility tree. This can improve the experience for assistive technology users by hiding purely decorative images.
    • You should use object-fit: cover; to the image which sets how the image should be resized to fit its container. object-fit: cover; maintains its aspect ratio while filling the element's entire content box.
    • Remember a modern css reset on every project that make all browsers display elements the same.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.

    Overall, great work! hopefully this feedback helps.

  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Thanuja Fernando,

    Excellent work! I have some suggestions regarding your solution:

    Consider using min-height: 100vh instead of height: 100% to the body , that let the body grows taller if the content of the page outgrows the visible page.

    • An explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • Remember a modern css reset on every project that make all browsers display elements the same. Set the image to display: block as there is a little gap under the image , you can see it when use devtools.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.

    Links must have discernible text also Check the footer's link , there are two nested links.

    After , you fix the issues, you can generate another report for your solution.

    Hopefully this feedback helps.

  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Welangai Eric,

    Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

    HTML

    • Page should contain <h1> . The <h1> is most commonly used to mark up a web page title. This challenge is supposed to be one component of a web page. To tackle the accessibility issue in the report , you may use an <h1> visually hidden with class=”sr-only”. You can find it here.
    • The most important part in this challenge interactive elements. Since there's a :hover state on the image and means it's interactive, So there should be an interactive element around it. When you create a component that could be interacted with a user , always remember to include interactive elements like(button, textarea,input, ..)

    for this imagine what would happen when you click on the image, there are two possible ways:

    1: If clicking the image would show a popup where the user can see the full NFT, here you use <button>.

    2:If clicking the image would navigate the user to another page to see the NFT, here you can use <a>.

    You should have used <a> to wrap Equilibrium #3429 and Jules Wyvern too.

    • The link wrapping the equilibrium image should either have Sr-only text, an aria-label or alt text that says where that link takes you.
    • For any decorative images, each img tag should have empty alt="" and add aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images in icon-view, icon-clock, icon-ethereum.
    • Profile images like that avatar are valuable content. The alternate text should not be avatar.You can use the creator's name Jules Wyvern. Read more how to write an alt text .
    • You should use <p> instead of <h4> in <h4> 0.041 ETH</h4>.
    • There are so many ways to do the hover effect on the image, The one I would use is pseudo elements::before, ::after. You can use pseudo-elements to change the teal background color to hsla. Then the opacity can be changed from 0 to 1 on the pseudo element on the hover. Also using pseudo elements makes your HTML more cleaner as there's no need for extra clutter in the HTML.
    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.

    CSS

    • Consider using min-height: 100vh instead of height: 100vh to the body , that let the body grows taller if the content of the page outgrows the visible page.
    • width:350px; an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • height: 600px; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • The icon view does not really need to be in the HTML. You can use CSS for it.
    • Remember a modern css reset on every project that make all browsers display elements the same. Set the image display: block ; as there is a little gap under the image, and that's way you have used height: 98%.
    • Last, Don’t Repeat Your CSS is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi VaporDusk,

    Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

    HTML

    • Page should contain <h1> . The <h1> is most commonly used to mark up a web page title. This challenge is supposed to be one component of a web page. To tackle the accessibility issue in the report , you may use an <h1> visually hidden with class=”sr-only”. You can find it here.
    • Don't capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .

    CSS

    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser.
    • You can use flexbox properties to the container that wraps the three card and give it flex-direction : row for the desktop and column for the mobile.
    • If you make each column into a flex column. Then set everything inside the cards to have some margin in one directionmarin-bottom: ; only the link wouldn't need it and use margin-top:auto on the learn more link that will push it to the bottom of the cards.
    • Add border-radius and overflow hidden to the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
    • It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • An explicit width is not a good way to have responsive layout . Consider using max-width to the component that wraps the three cards in rem.
    • Remember a modern css reset on every project that make all browsers display elements the same.
    • Don’t Repeat Your CSS(DRY) is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Riikis,

    Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

    HTML

    • Page should contain <h1> . The <h1> is most commonly used to mark up a web page title. This challenge is supposed to be one component of a web page. To tackle the accessibility issue in the report , you may use an <h1> visually hidden with class=”sr-only”. You can find it here.
    • You can upload the two sets of Google fonts in one CSS link , after selecting both font family, you can copy the CSS link that contains both font families.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.

    You absolutely did great job readable and reusable code.

    Hopefully this feedback helps.

  • PhoenixDev22•16,850
    @PhoenixDev22

    Hello Webdevsonu,

    Congratulation on finishing this challenge. Great job on this one! I have few suggestions regarding your solution:

    HTML

    • Never use <div> and <span>alone to wrap a meaningful content. Just keep in mind that you should usually use semantic HTML in place of the div tag unless none of them (the semantic tags) really match the content to group together. By adding semantic tags to your document, you provide additional information about the document, which aids in communication.
    • Page should contain <h1>. In this challenge, as it’s not recommended to have more than <h1>, you may use<h1> visually hidden with class=”sr-only”. You can find it here then you can use <h2> instead of a generic div.
    • Images must have alt attribute. In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • You have used <br> , using <br> is not only bad practice, it is problematic for people who navigate with the aid of screen reading technology. Screen readers may announce the presence of the element. This can be a confusing and frustrating experience for the person using the screen reader. You can read more in MDN.
    • It’s not recommended to use <br> to increase the gap between lines of text Or make empty space between elements, use padding / margin styling via CSS. And about using <br> in the <p> to make the paragraph break in new line, You may use max-width to <p> and remove those <br>.
    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.
    • Add border-radius and overflow hidden to the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • Consider using max-width to the component that wraps the three cards instead of setting to individual cards.
    • height: 350px; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • Remember a modern css reset on every project that make all browsers display elements the same.
    • Don’t Repeat Your CSS(DRY) is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Michael,

    Congratulation on finishing this challenge.

    Great job on this one! I have few suggestions regarding your solution:

    • About <h1>it is recommended not to have more than one h1 on the page. Multiple <h1>tags make using screen readers more difficult, decreasing your site’s accessibility. In this challenge, as it’s not a whole page, you can have<h1>visually hidden with sr-only. Then you can swap those <h1> with <h2>.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • In this challenge, what would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the <a. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.
    • To know when to use one or the other in a specific situation, you must understand that every action on site falls under two different categories:

    1. Actions where users affect the website’s back-end or front-end.

    2. Actions where users won’t affect the website at all.

    Action where users affect the website itself is where you use a button. For example, sign-up and purchase actions are often buttons. The user in these situations are creating a new account and completing a monetary transaction, which are actions that affect the website’s back-end. Creating new posts or making comments are actions that change a website’s content and what the user sees.

    Actions where users won’t affect the website are where you use a link. These actions that take users from one page to another without changing anything on the website’s back or front-end.

    • On your buttons, add border: 2px solid transparent; to the regular state. This way when the hover on the buttons , it doesn't add an additional 4 pixels to the height and width making the elements shift.
    • Add border-radius and overflow hidden to the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
    • Consider using rem for font size .
    • Remember a modern css reset on every project that make all browsers display elements the same.
    • It's recommended to include a git ignore file. This came with your starter files and is extremely important as you move onto larger projects with build steps

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi KL,

    Congratulation on completing another frontend mentor challenge.

    Great job! i have some suggestions regarding your solution:

    • You should use <main> landmark for the main body content and<footer> for the attribution as HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
    • About <h1>it is recommended not to have more than one h1 on the page. Multiple <h1>tags make using screen readers more difficult, decreasing your site’s accessibility. In this challenge, as it’s not a whole page, you can have<h1>visually hidden with sr-only. Then you can swap those <h1> with <h2>.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • The element button must not appear as a descendant of the a element. In this challenge, what would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the <a. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.
    • To know when to use one or the other in a specific situation, you must understand that every action on site falls under two different categories:

    1. Actions where users affect the website’s back-end or front-end.

    2. Actions where users won’t affect the website at all.

    Action where users affect the website itself is where you use a button. For example, sign-up and purchase actions are often buttons. The user in these situations are creating a new account and completing a monetary transaction, which are actions that affect the website’s back-end. Creating new posts or making comments are actions that change a website’s content and what the user sees.

    Actions where users won’t affect the website are where you use a link. These actions that take users from one page to another without changing anything on the website’s back or front-end.

    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.
    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser.
    • width: 62.5rem; an explicit width is not a good way to have responsive layout . Consider using max-width to the card.
    • height: 32.1875rem; - It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
    • Remember a modern css reset on every project that make all browsers display elements the same.
    • Don’t Repeat Your CSS(DRY) is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.

    Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Cassia Moraes,

    Congratulation on completing anther frontend mentor challenge. I have some suggestions regarding your solution:

    • You should use <footer> landmark for the attribution , it should live outside the the <main> as landmarks allow screen reader users to navigate through sections of your website by skipping to content that interests them. Landmarks could be seen as the logical layout of the website's UI, which is divided into e.g. header, navigation, main content, and footer. So the usage makes sense in any case.
    • Page should contain <h1> . The <h1> is most commonly used to mark up a web page title. This challenge is supposed to be one component in a web page. To tackle the accessibility issue in the report , you may use an <h1> visually hidden with class=”sr-only”. You can find it here.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • In this challenge, what would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the <a. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.
    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute, you can expose your site to performance and security issues.
    • Add border-radius and overflow hidden to the main container that wraps the three cards so you don't have to setborder-radiusto individual corners.
    • width: 375px;an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • Don’t Repeat Your CSS(DRY) is a good general principle to follow and eliminating duplication of css code should naturally be part of coding journey.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Aside these, your solution looks great. Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hello Real uNondaba,

    Well done! I have some suggestions regarding your solution if you don't mind:

    • About <h1>it is recommended not to have more than one h1 on the page. Multiple <h1>tags make using screen readers more difficult, decreasing your site’s accessibility. In this challenge, as it’s not a whole page, you can have<h1>visually hidden with sr-only. Then you can swap those <h1> with <h2>.
    • In this challenge, the images are much likely to be decorative. For any decorative images, each img tag should have aria-hidden="true" attribute to make all web assistive technologies such as screen reader ignore those images .
    • In this challenge, what would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the<a>. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.
    • On your buttons, add border: 2px solid transparent; to the regular state. This way when the hover on the buttons , it doesn't add an additional 4 pixels to the height and width making the elements shift.
    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.

    Overall , your solution looks great. Hopefully this feedback helps.

  • PhoenixDev22•16,850
    @PhoenixDev22

    Well done! I have some suggestions regarding your solution if you don't mind:

    • In my opinion, Add to cart is much likely to be a <button> with type="submit" instead of <a>, in a <form>. To know when to use one or the other in a specific situation, you must understand that every action on site falls under two different categories:

    1. Actions where users affect the website’s back-end or front-end.

    2. Actions where users won’t affect the website at all.

    Action where users affect the website itself is where you use a button. For example, sign-up and purchase actions are often buttons. The user in these situations are creating a new account and completing a monetary transaction, which are actions that affect the website’s back-end. Creating new posts or making comments are actions that change a website’s content and what the user sees.

    Actions where users won’t affect the website are where you use a link. These actions that take users from one page to another without changing anything on the website’s back or front-end.

    • I recommend to use <picture> tag in HTML to specify image resources. The <picture> tag contains<source>and <img> tags. This way the browser can choose the image that best fits the current view and/or device. If you have a small screen or device, it is not necessary to load a large image file. The browser will use the first<source>element with matching attribute values, and ignore any of the following elements.

    The alternate text in the product image should not be empty it should describes the product.

    • In HTML, the <del> tag is used to identify text that has been deleted from a document but retained to show the history of modifications made to the document. Strike through is a CSS property and does not carry any semantic meaning as inserted or deleted for screen readers. For screen reader: <del>deleted indicates text removed. In this instance, the two prices are read out which can be confusing.
    • The cart image in the button is a decorative image. For decorative images, you set an empty alt to it with an aria-hidden=”true” to remove that element from the accessibility tree. This can improve the experience for assistive technology users by hiding purely decorative images. Remember the alternate text should not be hyphenated, it should be human readable.
    • You should use object-fit: cover; to the image which sets how the image should be resized to fit its container. object-fit: cover; maintains its aspect ratio while filling the element's entire content box.
    • width: 800px; an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • height: 600px; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • It's recommended to set letter spacing values in em.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Overall, great work! hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Felix Makinda,

    Congratulation on completing your first frontend mentor challenge. I have some suggestions regarding your solution:

    • In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like QR code to frontendmentor.io not describes the image.
    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser. No need for position absolute and the negative margins.

    Personally, I don’t restrict the width of the body element. If I need to restrict the width I use a container div with a max-width on it.

    • width: 300px;an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • height: 600px; It's not recommended to set fixed height to component, you almost never want to set it. let the content of the component define the height.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Overall, your solution looks great. Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    hello ClassyKMR,

    Congratulation on completing your first frontend mentor challenge. I have some suggestions regarding your solution if you don't mind:

    • You should use <main> landmark to wrap the main body content as HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
    • In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like QR code to frontendmentor.io not describes the image.
    • Page should contain <h1>. In this challenge to tackle the accessibility issue, you may use<h1> visually hidden with class=”sr-only”. You can find it here. Then you can use <h2> instead of p class="block__text block__text--heading" as you should have used header ( typography ) to emphasize primary information in the card.
    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser. Remove those CSS from the html.
    • width: 280px;an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Aside these, great job. Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Mohammedsalih1,

    Congratulation on completing your first frontend mentor challenge. I have some suggestions regarding your solution:

    • You should use <main> landmark to wrap the main body content as HTML5 landmark elements are used to improve navigation experience on your site for users of assistive technology.
    • In my opinion, the image is an important content. The alternate text is needed on this image. The alternate text should indicate where the Qr code navigate the user : like QR code to frontendmentor.io not describes the image.
    • Page should contain <h1>. In this challenge to tackle the accessibility issue, as it’s supposed to be a part of a whole page, you may use<h1> visually hidden with class=”sr-only”. You can find it here. Then you can use <h2> instead of <h4> . Remember to use the headers in a chronological order.
    • In order to center the card on the middle of the page , you can use the flexbox properties and min-height: 100vh for the <body> add a little padding to the body that way it stops the card from hitting the edges of the browser. No need for position absolute.

    Personally, I don’t restrict the width of the body element. If I need to restrict the width I use a container div with a max-width on it.

    • width: 250px;an explicit width is not a good way to have responsive layout . Consider using max-width to the card in rem.
    • Consider using rem for font size , it' not recommended to use px for font size as absolute units don’t scale for example 15px will always be 15px on the same device. Using pixels is a particularly bad practice for font sizing because it can create some accessibility problems for users with vision impairments.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Overall, your solution looks great. Hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hello ch-andrew,

    Well done! I have some suggestions regarding your solution if you don't mind:

    • I recommend to use <picture> tag in HTML to specify image resources. The <picture> tag contains<source>and <img> tags. This way the browser can choose the image that best fits the current view and/or device. If you have a small screen or device, it is not necessary to load a large image file. The browser will use the first<source>element with matching attribute values, and ignore any of the following elements.
    • The alternate text in the product image should be more descriptive.
    • Specify the button type to avoid any unpredictable bugs.
    • It's not recommended to capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.
    • In HTML, the <del> tag is used to identify text that has been deleted from a document but retained to show the history of modifications made to the document. Strike through is a CSS property and does not carry any semantic meaning as inserted or deleted for screen readers. For screen reader: <del>deleted indicates text removed. In this instance, the two prices are read out which can be confusing.
    • The cart image in the button is a decorative image. For decorative images, you set an empty alt to it with an aria-hidden=”true” to remove that element from the accessibility tree. This can improve the experience for assistive technology users by hiding purely decorative images. Remember the alternate text should not be hyphenated, it should be human readable.
    • You should use object-fit: cover; to the image which sets how the image should be resized to fit its container. object-fit: cover; maintains its aspect ratio while filling the element's entire content box.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Overall, great work! hopefully this feedback helps.

    Marked as helpful
  • PhoenixDev22•16,850
    @PhoenixDev22

    Hi Muhammad Nur Ali,

    Congratulation on completing your first frontend mentor challenge.

    Great work on this challenge ! I have some suggestions regarding your solution if you don't mind:

    • In my opinion, Add to cart is much likely to be a <button> with type="submit" instead of <a>, in a <form>. To know when to use one or the other in a specific situation, you must understand that every action on site falls under two different categories:

    1. Actions where users affect the website’s back-end or front-end.

    2. Actions where users won’t affect the website at all.

    Action where users affect the website itself is where you use a button. For example, sign-up and purchase actions are often buttons. The user in these situations are creating a new account and completing a monetary transaction, which are actions that affect the website’s back-end. Creating new posts or making comments are actions that change a website’s content and what the user sees.

    Actions where users won’t affect the website are where you use a link. These actions that take users from one page to another without changing anything on the website’s back or front-end.

    • In my opinion, the image is an important content. As you have used CSS for the image, it’s not accessible anymore. The only method that is truly accessible and supported by nearly all browsers is to use inline images instead of background CSS images to display non-decorative content.
    • I recommend to use <picture> tag in HTML to specify image resources. The <picture> tag contains<source>and <img> tags. This way the browser can choose the image that best fits the current view and/or device. If you have a small screen or device, it is not necessary to load a large image file. The browser will use the first<source>element with matching attribute values, and ignore any of the following elements.
    • In HTML, the <del> tag is used to identify text that has been deleted from a document but retained to show the history of modifications made to the document. Strike through is a CSS property and does not carry any semantic meaning as inserted or deleted for screen readers. For screen reader: <del>deleted indicates text removed. In this instance, the two prices are read out which can be confusing.
    • The cart svg in the button is a decorative svg. For decorative svgs , you set an aria-hidden=”true” and focusable=”false to remove that element from the accessibility tree. This can improve the experience for assistive technology users by hiding purely decorative images.
    • There are so many arguments against setting the root font-size: 62%it state that you should never change the root font size because it harms accessibility.
    • You should use object-fit: cover; to the image which sets how the image should be resized to fit its container. object-fit: cover; maintains its aspect ratio while filling the element's entire content box.
    • To set letter spacing with CSS, use the em measurement unit.
    • Remember a modern css reset on every project that make all browsers display elements the same.

    Overall, great work! hopefully this feedback helps.