Responsive solution using HTML/CSS only

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Esteban!

  🎉 Congratulations on finishing this challenge! I notice that the link to the source code is not a valid URL (https://github.com/Estebankdb17/3column.previewcards.projecthttps:/github.com/Estebankdb17/3column.previewcards.project). It looks like there are two links in there.

  Anyway, I am able to see the source code, so here is my feedback:

  • Accessibility
    • 👍 Good job on taking care of those images!
    • 👍 Good job on using main and footer tags!
    • You don't need to wrap the link with section tag. I would recommend styling the link so that it looks like a button.
    • Use CSS to uppercase the text. The uppercased word in the HTML will be spelled by the screen reader (spelled letter by letter).
    • The hidden h1 should have text content. The purpose of h1 is used as a label of the page or identifier for the page. So, that's why every page should have one h1. You can write 3-column preview card component challenge by Frontend Mentor.
  • Styling
    • I would recommend specifying the main font family on the body element, becuase most of the elements will inherit the styling from the body element. By doing that, you can prevent yourself keep repeating the font-family properties.

  body {
    font-family: 'Lexend Deca', sans-serif;
  }
  

  • Instead of keep repeating the text-decoration: none; . I would recommend making all the anchor tags as text-decoration: none;.

  a {
    text-decoration: none;
  }
  

  • Best Practice (Recommended)
    • Remove the commented code. If another developer (it can be you in the future) wants to improve this solution, he/she might get confused about whether or not the commented code should be used or deleted.
    • Always use classes to reference all the elements that you want to style. Using id is going to make your stylesheet have high specificity (hard to maintain).

  That's it! Hopefully, this is helpful!

  Marked as helpful

  1

  ---

  

  Esteban Reyes•40

  @Estebankdb17Posted almost 3 years ago

  @vanzasetia Oops! Luckily you could saw the code anyway. Thank you for your time and your tips, the feedback is so important!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @Estebankdb17 You're welcome! Don't forget to fix the broken link! 😉

  1
• 

  Agata Liberska•3,995

  @AgataLiberskaPosted almost 3 years ago

  Hi @Estebankdb17, really nice work here, it's great that you remembered your landmark elements. Two things jump out that can be corrected relatively quickly:

  • In mobile layout, the border radius is exactly what it is in desktop layout - if you wrapped all three cards in a container, you could set the border radius on that container which would do the job for both column and row layout (you may need to add overflow: hidden to get it to show up)

  • In mobile layout, the footer is sort of in the middle of everything, can't figure out quite why right now, but - you could get rid of position: absolute on your footer, and use flex box properties to get it to stick to the bottom of the screen. To achieve this, you would need to set the body to flex-direction: column;' so that the mainis on top of thefooter. Then you need to make sure that the mainelement expands to fill all available space usingflex-grow: 1` (you may also need to set the max-width to 100% to get it to collapse correctly.

  • I think you could also simplify your markup and styles a bit - for example, your learn more anchor tags don't really need to be contained by anything, definitely not a section element, which really doesn't need to be a grid container.

  Hope this helps, let me know if you have any questions :)

  Marked as helpful

  1

  ---

  

  Esteban Reyes•40

  @Estebankdb17Posted almost 3 years ago

  @AgataLiberska Thank you Agata! Great suggestions, I noticed the footer issue just after I published the solution D:

  I'll keep this in mind for the next one!

  0