@GustavoSouza123
Submitted
@silkcoder
@GustavoSouza123
Submitted
@silkcoder
Posted
Congrats for completing the task.
I noticed one thing, You applied the style height:100vh
to the container element. Its good but min-height:100vh
is more suitable here.
To see why, open your site on google chrome, then open developer tools, then click on mobile view, then choose Iphone 12 pro, the site looks good but if you change the orientation, you will see some white colored space at bottom. You can fix this by applyingmin-height:100vh
instead of height:100vh
to the element with class 'contianer'.
Good luck with your future tasks.
Marked as helpful
@mohamedqabbari
Submitted
I've just completed a front-end coding challenge from @frontendmentor! π
You can see my solution here: https://www.frontendmentor.io/solutions/blog-preview-card-challenge-by-sass-WDgxOYxpoM
Any suggestions on how I can improve are welcome!
@silkcoder
Posted
Congrats for completing the task.
I noticed one thing, you applied flex properties to the HTML tag to center the 'card' class element.I think you should apply it to the body tag rather than applying to the HTML tag.
It's generally considered a good practice to put styles in the <body> element
@SumathiTrishna
Submitted
@silkcoder
Posted
Good attempt.
You can place your container in the center of the screen vertically by changing the height property of the .container class
change min height:600px to min-height:100vh
.container {
min-height:100vh
}
Now the container get placed vertically centered on the screen.
Good luck.
@SumathiTrishna
Submitted
@silkcoder
Posted
Hello there,
Your solution looks very good, I have a few suggestions.
The container is not centered vertically, to place the container vertically center, just add the following style to your .container class
.container {
//add this property to your existing styles
min-height: 100vh;
}
Now you can remove margin-top property from .box class
.box {
//remove this style
margin-top: 50px;
}
I checeked the repo and I think its good to place your assets in separate folders, like images goes to 'images' folder and css files to you a 'css' folder.
It is not a good practice to add inline styles to an HTML element (you added inline styles to a div with class box)
Good luck with your future tasks
Good day
@Ezekiel225
Submitted
@silkcoder
Posted
Congrats for completing the task.
I have two points to make this layout better.
I am using a wide monitor and the background image looks broken. You can make it better by adding following styles to your body tag
body {
//your exisitng styles
background-repeat: repeat-x;
}
Once you add this styles, the background image will cover the entire screen.
When I open your page, I can see a scroll bar in the layout, its because body tag has default margin. As a quick fix, you can add margin:0px to your body tag
body {
//Your existing styles
margin:0px;
}
Now you will see the layout without a scroll bar
Its recommended to have a reset.css file in the project to reset all the default styles. You can find sample reset file at https://meyerweb.com/eric/tools/css/reset/
Good luck with your future tasks
@ghost237-sys
Submitted
@silkcoder
Posted
Hello there,
Congrats for completing your task, It looks good, however I have a few suggestions that can make your layout better
First thing you have to keep in mind is that your layout get viewed in different sized devices. so is not a wise decision to set width for body tag. so you can remove width property from body tag
body {
//remove this
width: 1440px
}
Next, you can remove margin property you give to container div (we have a better method to center the container :) )
.container {
//remove following line
margin: 200px 250px 150px 580px;
}
Now the container get aliged to top left, that is not good :) we have to place the container centered in all sized devices. so apply the following propertiese to the body tag in addition to the existing styles
body {
//your styles here
display: flex;
justify-content: center;
align-items: center;
min-height: 100vh;
}
Now the container get aligned to centered horzontally and vertically. Good job
you can read more about flex box propertise at https://www.geeksforgeeks.org/introduction-to-css-flexbox/
Now if you notice, you may see a scroll bar in the window, its because there is a margin property get added to body tag by default. so a recommended method to fix this is to use a reset css file
[this is a sample reset css file] (https://meyerweb.com/eric/tools/css/reset/)
you can download and load this css before loading your styles.css, this file reset all default styles get added to the HTML document
As a quick fix (without using a reset file) you can add 'margin:0px' to your body tag to remove the scroll bar
body {
//all your styles in body tag goes here
margin:0px;
}
Now your layout look better.
I noticed that on your repo, all files (images, css) are scattered, Its good to keep them on separate folders, so that all your images will stay on images folder and your styles to a styles folder
Good luck with y our future tasks
Good day
@Jamesolukanni
Submitted
@silkcoder
Posted
Your solution looks good
I have a suggestion about the max-width property you given to body tag.
If your solution opened in a screen with width greater than 1440px, the content not get centered. So you can remove the max-width property of body tag
body {
//remove
max-width: 1440px;
}
Once you remove this property, your content remains on cener for all sized devices.
Another point
To center the container its wise to use Flexbox properties, read more about it on the page https://www.geeksforgeeks.org/introduction-to-css-flexbox/
Marked as helpful
@Adidans
Submitted
@silkcoder
Posted
Congrats for completing the task
I noticed a few things
Good luck with your future challenges. Have a good day
@meriamBenaliFellague
Submitted
@silkcoder
Posted
Congrats for completing the task, it looks good.
However, I have a suggestion to place the main tag content in the center of the screen horizontally and vertically.
In your style.css file, add the following styles to body tag
body {
//add following styles to your existing styles
display: flex;
justify-content: center;
align-items: center;
}
remove 'margin-left: 35%;' and 'margin-top: 13.5%;' from your main tag styles
main {
//remove the following styles
margin-left: 35%;
margin-top: 13.5%;
}
Now the main content get centered in all sized devices.
You can read more about flex box styles in the following page Introduction to CSS Flexbox
Good luck with your future challenges. Have a good day.
Marked as helpful
@MelvinAguilar
Submitted
π Features:
π οΈ Built With:
Any suggestions on how I can enhance this solution or achieve even better performance are welcome!
Thank you. πβοΈ
@silkcoder
Posted
Congrats, good job
@dashaunn
Submitted
@silkcoder
Posted
Good work.
@swapuruguay
Submitted
Frontend Mentor - Product preview card component solution
Technologies:
@silkcoder
Posted
Congrats for completing the task.
I am adding a few points to improve your layout
I also completed the same task today, have a look and let me know your comments, thanks
All the best
Marked as helpful