Frontend Mentor | Ahmad Syafiq Aqil Wafi's profile

All comments

@ACdev27Submitted over 3 years ago

Any feedback is appreciated. The design files only showed a desktop and mobile view. In cases like that is it good practice to also make sure the the "tablet" view looks good too?

Stats preview card component - flexbox, css variables

Ahmad Syafiq Aqil Wafi•170

@SyafiqjosPosted over 3 years ago

Hi @ACdev27,

Congratulations on finishing the challenge!

I checked the live website, it's working great and responsive but I have some opinion about them.

The card is seems to be really wide on max-width of 757px. The user who read the text probably got tired to read all the long from left to right. Maybe try to shrink it? so the text appear easy to read from top to bottom.
I see that you using manual margin for the stats. It's great but, there is other efficient and fast approach.

Change from this..

.stats {
  display: flex;
  justify-content: flex-start;
  margin-top: 18%;;
}
.stats div {
  margin-right: 15%;
}
.stats div:nth-child(3) {
  margin-right: 0;
}

Into this..

.stats {
  display: flex;
  justify-content: space-between;
  margin-top: 18%;
}
.stats div {

}
.stats div:nth-child(3) {

}

Also if you still want to use your current approach it's fine, but it's easier to change div:nth-child(3) into div:last-child to make it even more versatile.

Also I think it isn't necessary to put alt in the image, you could leave it empty alt="", since it's not required for us to describe it. Usually alt is used to describe or make a more explanation to the user based on the paragraph or text. But in this challenge the image is only appeared as a background image, there is no relation between the text and stats. It's better that you use role="presentation" or aria-hidden="true" to make screen reader ignore that image.
Last but not least, I think there is slight typo on the queries stats. It's should be "12M+" instead of "12m+".

Thats all from me, have a good day and keep improving!

syafiqjos

Marked as helpful