Looking to hire developers?
@DimitriTsikaridze
@EliasA-com
Submitted
Hello, I'm new to web development but I have a basic understanding of HTML/CSS. However, I feel that there might be simpler ways of making this challenge. I would greatly appreciate any advice or feedback on my code to make it more efficient and streamlined.
@DimitriTsikaridze
Posted
hi, @EliasA-com good job for finishing this challenge. here are couple of things that could be improved in your solution.
try avoiding setting fixed widths and heights, especially heights, they can be pain to work with when you try responsive design, instead try to set max-widths and for heights let the layout do the work, no need to set fixed height.
for example you don't need to set height for .wrapper element. and for image you can replace hard-coded width value with max-width: 100%, this will make image responsive and make image as big as its parent container. it is also good idea to set img {display: block} as well since images are inline-block by default and they create little spacing beneath them. setting to display block will fix that.
use CSS shorthand! this will make CSS lot easer to read and write. for example when you're setting margin-left: 15px; margin-right: 15px; you can simply write this in one line using CSS logical properties.
margin-inline: 15px; this will set margin left and right to 15px;
margin-block: 15px; this will set margin top and margin bottom to 15px;
or simply use margin: 15px; to set equal margins to all sides. you can read about logical properties here:
and about CSS shorthand properties here:
good luck!
Marked as helpful
@BssiN
Submitted
@DimitriTsikaridze
Posted
hello @BssiN, good job for finishing this challenge. here are couple of things that could be improved in your solution.
it's generally good practice to keep styles in separate files and import them in html with link tag
<link rel="stylesheet" href="style.css" />
try avoiding setting fixed widths and heights, especially heights, they can be pain to work with when you try responsive design, instead try to set max-widths and for heights let the layout do the work, no need to set fixed height.
for example you don't need to set height for .wrapper element. and for image you can replace hard-coded width value with max-width: 100%, this will make image responsive and make image as big as its parent container. it is also good idea to set img {display: block} as well since images are inline-block by default and they create little spacing beneath them. setting to display block will fix that.
and finally there are some redundant and non-semantic html tags which can be deleted entirely. for example you don't need div .wrapper and .container elements. you can wrap entire card layout in 1 element preferably <main> tag or something semantic.
good luck!
@Betelhem-hailu
Submitted
@DimitriTsikaridze
Posted
Hi @Betelhem-hailu, good job on finishing this challenge. there are couple of things that could be improved in your solution. for example: try fetching data when component loads with useEffect that way you don't get empty box; empty array in second argument tells react to call this function when component mounts;
useEffect(() => {
fetchData()
}, []);
also fonts don't match and that can easily be changed
body {
font-family: Manrope; //or whatever font is in challenge
}
try setting cursor: pointer on button elements to indicate that they're clickable
button {
cursor: pointer
}
good luck!
Marked as helpful
@aniket22n
Submitted
Finally done with this challenge, please check and do let me know your thoughts on this one.
@DimitriTsikaridze
Posted
Hi Aniket, your website seems okay but there are few ways to improve your solution.
couple of things: don't use position relative and left/top properties for everything to position elements. instead use margins or paddings to adjust spacings between elements.
if you go this route you can remove default margins/paddings and set box-sizing property to border box, this simple css reset makes everything easier to style.
don't use capital letters for css class names.
there is no need to put text inside button in <p> tag. you can simply do <button>text</button> and that will work just as well. you might also want to add cursor:pointer to button elements to indicate that they're clickable.
hope you learn something new. keep up the good work!.
Marked as helpful
@anubhav1206
Submitted
@DimitriTsikaridze
Posted
Hi @anubhav1206,
you can give image elements max-width: 100% to make them responsive and not overflow the parent element.
.container img {
border-radius: 0.625rem;
margin-bottom: 1rem;
display: block;
max-width: 100%;
}
also instead of div with container class you could replace that div with main html5 element to be more semantic with your tags
happy coding.