Responsive page using CSS Flexbox

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted almost 2 years ago

  Hello

  Definitely change these to h2s. In this case you are doing a single component challenge, not a full Web page, so it's fine to have no h1 on the page if you want. If this component was on a real full Web page there would need to be a h1 higher up on the page.

  Learn more would almost certainly navigate not perform an action like open a modal or toggle content. So use anchor tags

  Don't use huge paddings to create your layout

  Remove width from the cards. All this component needs is a max width. The cards currently overflow the sides of my mobile screen because you have given them an explicit width, meaning they can't shrink if they need to.

  Other than these small things, all looks great to me

  One minor suggestion - line height is usually unitless. Nothing wrong with using rem, it's just become a convention to write a unitless value like 1.5. Your choice though, no harm that I know of.

  1

  ---

  

  Albina S•130

  @albina0104Posted almost 2 years ago

  @grace-snow Hello, thank you!

  1. Changed headings to <h2>.
  2. Changed buttons to links. But I wonder, why do we care about tags, but not about visual style? Why do links can look like buttons? By the way, in the "style-guide.md" document in the colors description it says "buttons": Very light gray (background, headings, buttons): hsl(0, 0%, 95%).
  3. In a jpg design file of a mobile version, there were empty spaces on the top and the bottom with 88px height, so I repeated that. Now removed.
  4. Changed to max-width, thanks!
  5. I used unitless values for line-height in previous challenges. But in this one, I measured all values in pixels with a screen ruler, including line height, and I thought the easiest way would be to convert it to rem, rather than guess the unitless line height.

  0
• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  Hello again 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  • It is generally not recommended to use multiple <h1> tags on a single webpage because the <h1> tag is used to mark the most important heading on a webpage and it is considered the top-level heading in the document outline. It should be used only once on the page, typically for the title or main heading of the page.

    Additionally, search engines use the heading tags, including the <h1> tag, to understand the structure and the main topic of a webpage, and using multiple <h1> tags can confuse the search engine, and it can negatively impact the SEO of a webpage.

    Source: The A11Y Project - Accessible heading structure

  • The same discussion as before, do the "Learn More" buttons perform any action or redirect the user to another website?

  I hope you find it useful! 😄

  Happy coding!

  0

  ---

  

  Albina S•130

  @albina0104Posted almost 2 years ago

  @MelvinAguilar thanks for your reply!

  1. Do you mean I need to change all 3 headings to <h2>? Is it okay or not if there will be no <h1> on this page?
  2. Again, I am confused by their visual look... Maybe they should be links instead of buttons. But in these challenges, nothing tells us what happens when we click on them - what if they don't go to another page, but open a pop-up?

  0

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted almost 2 years ago

  @albina0104 The correct thing to do is to use h2 for the headings, and you should use an h1. You can create an '<h1>' element that will be hidden visually but visible and readable by screen readers. The class "sr-only" hides content visually and here are the styles to copy. e.g.: <h1 class="sr-only">3-column preview card component</h1>

  That is correct, there is nothing telling you which action you should use, for opening a modal a button is better and for redirecting to another site a link, in my humble opinion, until you implement a modal or an action a link could be better.

  0