Responsive page with flexbox and media queries

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  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,400

  @correlucasPosted about 2 years ago

  👾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

  @DavidMorgadePosted about 2 years ago

  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
• 

  Edwin Jonathan•90

  @ElektrokatPosted about 2 years ago

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

  0