Using css grid, background-blend-mode and sass.

Community feedback

• 

  Srikar Gunnam•150

  @srikargunnamPosted almost 3 years ago

  Hi Alexandro Munera,

  First I would like to congratulate you for completing this challenge 🎉

  I would like to give few suggestions based on my observations.

  1. It is must to use h1 tag (level one heading) as per web accessibility standards, you can swap h2 with h1, and my advice is to use a header tag instead of a div tag to wrap h1 and p tags.

  2. Your card looks stretched, that is because you used width as 90%, so there is no max-width, Instead of using width as 90% use min(90%, "the max width the content fits good"), so that your content looks good even or much wider screens.

  3. Breakpoint for mobile screens mentioned in the style guide is 375px, but used a breakpoint at 768px, it would be better if you can work on that, i can understand that the content looks like weird when it reached 768px, for this you could as a media query for tablet assuming it is given in this project, it will give you a new opportunity to work on your own ideas to make the content suit tablet view until it reach 375px.

  4. I have seen that you have used rem units at some places and px units at some places, it is a good practice to stick to one type of units, i would suggest to go with rem entirely.

  This link "https://www.sitepoint.com/understanding-and-using-rem-units-in-css" will help you get a better understanding of rem units, and how you can easily use a trick to make 10px = 1rem, this will help you easy to calculate and use rem units, but keep in mind that this trick will not work for media queries which is not mentioned in the above link.

  hope this helps 😉, please mark as helpful if it does.

  Marked as helpful

  1

  ---

  

  Freud•210

  @AlexandroMuneraPosted almost 3 years ago

  @srikargunnam Thank you so much, exelent that trick to work with 10px = 1rem and the function min().

  0

  

  Srikar Gunnam•150

  @srikargunnamPosted almost 3 years ago

  Hi @AlexandroMunera, I forgot to mention that the trick with 10px = 1rem works only for font-size, sorry if i misguided you by not mentioning this.

  Happy coding 😉

  1
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello @AlexandroMunera,

  I have some suggestions regarding your solution :

  • Page should contain a level-one heading , so you can add a <h1> with class="sr-only" (Hidden visually, but present for assistive tech).

  .sr-only {
  	border: 0 !important;
  	clip: rect(1px, 1px, 1px, 1px) !important;
  	-webkit-clip-path: inset(50%) !important;
  		clip-path: inset(50%) !important; 
  	height: 1px !important;
  	margin: -1px !important;
  	overflow: hidden !important;
  	padding: 0 !important;
  	position: absolute !important;
  	width: 1px !important;
  	white-space: nowrap !important;            
  }
  

  • Never use <span> 's or <div>'s for meaningful content.

  • Forclass="card__stats-list" , it would be better if you use an unordered list<ul> with 3 items for the stats.

  • The number and word have to be read together to make sense so need to be in the same meaningful element. so only a span or maybe strong tag needs to wrap the numbers. the words like companies should not be in paragraph tags. They don't need to be wrapped in anything as they are already inside a meaningful element (list item)

  Overall , your solution is good, Hopefully this feedback helps.

  Marked as helpful

  1

  ---

  

  Freud•210

  @AlexandroMuneraPosted almost 3 years ago

  @PhoenixDev22 Thanks Phoenix, I understood the importance of h1 tag and the use of ul tag.

  1