Skip to content
OverviewSolutions
14
Comments
16

Josh

@burrijwSalt Lake City, Utah400 points

Father | Utah, USA | Web Developer | Sports | Consumer Tech | Utah Jazz | Music | Comedy

I’m currently learning...

React, Next.js, and always trying to get better at Accessibility. I'm also learning MySQL, Postgres, and the Drizzle ORM.

Latest solutions

View more solutions

Latest comments

  • Josh•400
    @burrijwPosted over 1 year ago

    Hey, nice work.

    Consider using a list for the statistics. Any time you have similarly-shaped data or UI elements, it might make sense to be in a list.

    Marked as helpful
    1
  • Josh•400
    @burrijwPosted over 1 year ago

    Hey, good start!

    There's nothing wrong with using an "extra" div if it has a purpose. Ain't nothing wrong with a div as long as you are using it with intent. You actually do need some kind of wrapper element around the three sections so that you can change the way they are laid out based on the screen. You want them stacked vertically on mobile and horizontally on desktop, so you need some way of controlling that. I'd use flexbox, personally.

    • Make sure you have all your primary page content in a <main>.
    • Use a <footer> for the attribution.
    • Try setting the border-radius on the wrapper with overflow set to hidden instead of trying to manage the border-radius of each individual card.

    Good luck! Refactor this and submit it again! 😄

    Marked as helpful
    2
  • Josh•400
    @burrijwPosted over 1 year ago

    Hi! Welcome to Frontend Mentor. You're off to a good start, but there's a lot that can be improved.

    HTML

    • Include a <main> element on every page that you make. Landmarks are an important part of semantic HTML and help a lot with accessibility. Check out this MDN page
    • The structure of this component (and challenge overall) is quite simple. And you want to keep your HTML as simple as possible to improve performance and accessibility. All you really need is this:
    <body>
<main>
<img>
<h1></h1>
<p></p>
</main>
<footer>
...
</footer>
</body>
    • You've used a <strong> tag where you should probably use a heading. Avoid using <strong> and similar tags based on the visual state of something. Think more about the semantic meaning instead.

    CSS

    • Generally, you should be working mobile-first and adding overrides as the screen gets bigger. But this challenge doesn't require anything like that. It's the same at all screens.
    • Don't set font sizing in px, use rem instead.
    • It's hard to read the attribution. Change the text color to something with contrast against that background color.
    • Try using rem for your border radius values.

    Make some changes and submit this one again! Refactoring is where the real learning happens. And come join us on Discord if you haven't already!

    Marked as helpful
    1
  • Josh•400
    @burrijwPosted almost 2 years ago

    This looks really good. ✨ Just a few things mostly related to your HTML that could use some improvement.

    • You need a <main> element to wrap your main content. This helps with accessibility and ensures that all content is properly structured.
    • The image at the start of the show should have an appropriate alt text. "Equilibrium" isn't very descriptive and doesn't give any information about the image. Make sure you use concise but descriptive text to describe the image.
    • The "view" icon is interactive, so it needs to be a <button> or <a> element in order to have hover styles. Since it likely opens a larger version of the image in a modal, a <button> would be the best choice here. Additionally, it should be focusable by keyboard as well as clickable with a mouse, so that all users can access the interaction.
    • To make the icons accessible, you can try hiding them from screen readers. This can be done by setting the content to an empty string and using background-image to set the image. Alternatively, you can use the images inline and leave the alt attribute blank. WAI has a great tutorial on accessible images that you can check out here: https://www.w3.org/WAI/tutorials/images/.
    • You may also want to look into how you can better describe the ETH price of the NFT to someone using a screen reader. Could some visually-hidden text around it help clarify what it is? Maybe aria-description?

    Keep up the good work!

    Marked as helpful
    1
  • Josh•400
    @burrijwPosted almost 2 years ago

    Nice 👍🏻

    A couple things:

    • You need one (and usually only one) <main> on each page of a website.
    • Overall you could probably simplify your HTML a bit. Nothing major, but there's no need for the <header> inside the card, for example. Keep it simple.
    • Check your padding and border-radius values. I think you could get closer to the design with a bit of tweaking. No need to be pixel-perfect, but you could be closer, IMO.

    Great job -- keep it up!

    Marked as helpful
    1
  • Josh•400
    @burrijwPosted almost 2 years ago

    HI 👋🏻 Nice start!

    You've got some HTML/a11y issues:

    • The nav isn't keyboard operable. Make sure you follow patterns like this APG example when implementing interactive widgets and things.
    • I don't think <aside> is the right element to use in this case. Don't let the visual form of a component dictate the HTML elements you choose. Just because a mobile navigation drawer is a sidebar, doesn't mean you should use <aside> just because it's meant for "sidebar" content. Think about the semantics. You can just use the <nav> without nesting it in another landmark. All you really have on this page for landmarks are the <header>, <nav>, and <main>.
    • Consider if you really need to duplicate the navigation in your HTML. Can you just style it differently without duplicating the markup?
    • Focus styles could be better. Right now what you have is hard to see when the black "Learn More" button is focused. Maybe offset the focus ring.
    • I'd place those image/logos at the bottom in a list.

    Good stuff. Keep it up!

    1
View more comments