Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Responsive page with flexbox and media queries

@Elektrokat

Desktop design screenshot for the 3-column preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


Hello guys, i just completed this challenge. Please I need your reviews, I didn't really have any issue with this but is there a better way it could have been done? Thank you in anticipation...

Community feedback

PhoenixDev22 16,950

@PhoenixDev22

Posted

Hi Edwin Jonathan,

Congratulation on completing this challenge. Excellent work! 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 swap those <h1> with <h2>.
  • In my opinion, the images are much likely to be decorative. For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images.
  • 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>. The type of the button is button not submit.
  • 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.

Marked as helpful

0
Lucas 👾 104,420

@correlucas

Posted

👾Hello Edwin, congratulations for your new solution!

I saw your code and I've two tips for you to improve it, first you can improve the semantics replacing the div with an articlefor all vehicles type card block. Then you can create a general class to manage all cards together with the properties that are the same like padding, margin, gap and border-radius and use a different class for the characteristics that unique like colors/icons. This way you can control everything with the same class.

👋 I hope this helps you and happy coding!

Marked as helpful

0
David 8,000

@DavidMorgade

Posted

Hello Edwin, great job getting the solution for this challenge! congratulations!

There is just a thing that I would like to help you because it seems that the responsiveness is failing a bit cause of that, let me explain my self.

Your media queries should be refactore, for example, your first media querie goes from 0 to 375px, @media: (max-width: 375px) this will only target a few mobile sizes screen, what you really want to target every type of mobile screen is @media(max-width: 768px), with this you will keep the one column layout until 768px instead of 375px. The other media querie is the one for the desktop sizes, you are using @media screen and (min-width 376px) and (max-width: 1440px) the problem with this is that screens with more than 1440px (a 2k monitor is like a standart in this days) will completely broke the styles!, you should probably switch to @media(min-width: 768px), with this, your mobile styles will keep until 768px and then switch to desktop from 768px to infinity!

I hope that you undestood my explanation, if you have any questions regarding this, don't hesitate to ask!

Marked as helpful

0

@Elektrokat

Posted

Thank you, guys, for the reviews, I really appreciate, will work on it...

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord