Skip to content
OverviewSolutions
14
Comments
16
Josh
@burrijw

All comments

  • Josh•400
    @burrijw

    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
  • Josh•400
    @burrijw

    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
  • Josh•400
    @burrijw

    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
  • Josh•400
    @burrijw

    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
  • Josh•400
    @burrijw

    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
  • Josh•400
    @burrijw

    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!

  • Josh•400
    @burrijw
    • The error state stays on the input after submitting a successful email. That's a bit confusing from a UX perspective.
    • The footer isn't visible on mobile without scrolling, even when there is plenty of room for it.

    Nice job!

  • Josh•400
    @burrijw

    Nice job! 👏🏻 This is a really good start.

    Here are some suggestions: Simplify your HTML. The simpler your markup, the easier your code will be to maintain. Keep it simple. All you need is this. 👇🏻

    <body>
  <main class="card">
    <img/>
    <h1>...</h1>
    <p>...</p>
  </main>
</body>
    • You don’t need a <picture> element in this case. Use that when you need to serve different image resolutions at different screen sizes, or when you need different images for artistic purposes.
    • Every webpage needs a <main> landmark. With something this simple, your whole card can be the main , but in a larger site you'd have many many elements inside and the main would contain everything except for a header and footer . You can read up on semantic HTML on web.dev.

    Avoid using ID's for CSS selectors. They have a very high specificity and it's going to cause more problems than it is worth. Use classes to avoid “specificity wars”.

    NEVER use px for fonts. Use rem or another responsive unit. You can read more on Grace's blog.

    Centering your card. There are some pretty simple ways of centering things in modern CSS. Here is the snippet I tend to use:

    body {
  display: grid;
  place-content: center;
  min-block-size: 100vh;
}

    Some will prefer using flexbox, though it is technically more code:

    body {
  display: flex;
  justify-content: center; 
  align-items: center;
  min-block-size: 100vh;
}

    Some nitpicky things:

    1. The background color is off.
    2. The heading font is not the right size.
    3. You have some typo’s: container in your CSS and skills in your markup.

    Keep it up. You're off to a great start. :)

    Marked as helpful
  • Josh•400
    @burrijw

    Nice job! 👏🏻

    Here are a couple suggestions:

    • I'd make sure those social media icons at the bottom are links. Wrap the images in anchors and make sure they have accessible labels. You can hide the label with an 'sr-only' or 'visually-hidden' class.

    • The numbered sections are decorative, meaning they should also be hidden from screen readers. Consider adding an 'sr-only' class on those as well.

    • I think you can get rid of some of the hacky margin and positioning stuff if you use an SVG for the curved section backgrounds instead. Consider adding an ::after pseudo-element to those sections with a curved SVG as the content or background-image.

    • Hiding overflow on the body and/or HTML can be tricky. It won't work on mobile without some special care. I'd recommend avoiding it if you can.

    Good luck!

    Marked as helpful
  • Josh•400
    @burrijw

    Nice start! There's a number of things that could be corrected on this one, but I would actually recommend going back to the QR Code challenge first. You can always come back to this when you've got a little more experience. Best of luck!

  • Josh•400
    @burrijw

    Hi Isaac, this is a great start! There are a couple accessibility issues you should address.

    • You need a <main> tag and all content should be inside landmarks where possible. Landmarks on W3Schools
    • You should have the image in your markup. It's meaningful to the page content because it's showing the product described by the text. Try adding it with the <picture> element to serve different images responsively and have an alt="" attribute.
    • Use a button for the "Add to Cart" button. Never use a div as an interactive element.
    Marked as helpful
  • Josh•400
    @burrijw

    Good start!

    • Check your <link> to the normalize.css file. There's an error: "space is not allowed".
    • There’s horizontal overflow at 320px wide. This is inaccessible per the WCAG 2.1.
    • You have a lot of things wrapped in <div> tags unnecessarily. Try to only use them when you need to for styling or scripting.
    • You don't need that div for the line. It's possible to use a border on the element before or after it instead. Or use a pseudo-element to keep your markup clean.
    Marked as helpful
  • Josh•400
    @burrijw

    Nice job!

    There’s horizontal overflow at 320px wide. This is inaccessible per the WCAG 2.1.

    “Content can be presented without loss of information or functionality, and without requiring scrolling in two dimensions for:”

    • “Vertical scrolling content at a width equivalent to 320 CSS pixels.”
    • “Horizontal scrolling content at a height equivalent to 256 CSS pixels.”

    The <h4>Annual Plan</h4> should be some other element. Heading elements should be used as headings for sections of content, not for text that appears to be bolder, larger, etc. You can use CSS for appearance. From MDN:

    • “Do not use heading elements to resize text. Instead, use the CSS [font-size](https://developer.mozilla.org/en-US/docs/Web/CSS/font-size) property.”
    • “Do not skip heading levels: always start from <h1>, followed by <h2> and so on.”

    You’re missing hover states on the “Change” and “Cancel Order” links.

    The background image doesn’t meet the width of viewports over 1440px.

    It looks like the background image isn’t changing between mobile and larder breakpoints. You can use a media query to change it.

    I think some spacing is off between mobile and larger viewports. Another opportunity to use media queries.

    Marked as helpful
  • Josh•400
    @burrijw

    Hi Caleb! This is a really, really strong start! 💪🏻

    • Make sure you have landmarks, especially <main> .
    • You can get away with less markup. No need to wrap things in a div unless it’s for a particular purpose. All you need on this one is:
    <body>
  <main>
    <img />
    <h1>...</h1> 
    <p>...</p>
  </main>
</body>
    • Use padding for space inside of a component, and margin for space around it. You have margin around the image when all you really need is a consistent padding inside the card.
    • Font sizing might be a little off to my eye. Double check it if you care to. 🙂
    Marked as helpful
  • Josh•400
    @burrijw

    Hey! Nice attempt!

    Look into using the <picture> element for serving different images responsively. Here is a guide on MDN for responsive images. (Index.html : 21-22)

    It may not be the best fit semantically to us an article inside the card. From MDN: “The <article> HTML element represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable (e.g., in syndication).” It wouldn’t make sense to repurpose the card content without the product images. So the whole card may fit in an <article>, but the text content by itself might not. You could just use a <div> since the only reason for grouping those elements together is for styling.

    I would suggest revisiting your heading levels. I don’t think the “perfume” really serves as a page title.

    Try the <del> element for the old/previous pricing. The <strike> tag isn't recommended anymore.

    Marked as helpful
  • Josh•400
    @burrijw

    Looks good!

    When it comes to centering the tooltips, I used position: absolute on the tooltips and then did the ol' left: 50% translate-x: -50% trick to center them relative to the bar div. Maybe play around with that idea.