Skip to content
OverviewSolutions
33
Comments
1141

PhoenixDev22

@PhoenixDev2216,850 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • PhoenixDev22•16,850
    @PhoenixDev22Posted over 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,850
    @PhoenixDev22Posted over 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,850
    @PhoenixDev22Posted over 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,850
    @PhoenixDev22Posted over 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,850
    @PhoenixDev22Posted over 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,850
    @PhoenixDev22Posted over 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
View more comments