Skip to content
OverviewSolutions
33
Comments
1151
PhoenixDev22
@PhoenixDev22

All comments

  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    1
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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.

    0
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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.

    1
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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.

    0
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    0
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    1
  • Riikis•20
    @RikvdevSubmitted about 2 years ago

    I tried to make this with dinamic grid using auto-fit , without media queries, but I had some errors with padding and container width, so I did it with media queries.

    Did I do it right?

    Are there any errors?

    Where could I improve to make it better?

    I would like to read your opinion :)!

    3 Columns preview card using CSS Grid.

    2
    PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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.

    0
  • Webdevsonu•270
    @WebdevsonuSubmitted about 2 years ago

    Here another preview cards I found it easy to make for the desktop screen size, but I found a little difficulty to make it switch the border-radius for small screen size as it shrinks. And I used "flex" to arrange the cards but I found out it wasn't perfect at some point while shrinking. So will you suggest some easy methods to fix it and how to auto adjust those border-radius for small screen.

    3 Column flexible preview card component, using HTML and CSS

    1
    PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    0
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    1
  • Kaung Lun•110
    @lunk-kmlSubmitted about 2 years ago

    I think I make it look as close as possible. I still think my code is a little messy.

    Feel free to review my code and preview the site. I welcome any feedbacks, tips, or ideas. I'm still learning.

    !! Please let me know if my code is a mess or not lol

    3-column preview card component

    1
    PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    1
  • PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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
    1
  • Real uNondaba•90
    @NondabaSubmitted about 2 years ago

    Aligning the attribution class at the bottom was a challenge. How can I place attribution at the bottom while using flex box?

    Flexbox

    1
    PhoenixDev22•16,950
    @PhoenixDev22Posted about 2 years ago

    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.

    1