Mohamed Ibrahemโข 20
@moibrahem98
Submitted
any feedback about my best practice
Renszo Camacho
@RenszCamacho
All comments
- Renszo Camachoโข 1,615
@RenszCamacho
Posted
Hiya ๐๐ป moibrahem98.
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐, place properly the background image is quite tricky.
Just a few suggestions IMHO.
-
Add an alternative text to your images.
<img class="person" src="images/image-victor.jpg" alt="victor" /> -
I would add a CSS reset, to avoid some annoying margins.
* { margin: 0; padding: 0; box-sizing: border-box; }- To place the background-images:
body { background-image: url(../images/bg-pattern-top.svg), url(../images/bg-pattern-bottom.svg); background-position: right 50vw bottom 50vh, left 50vw top 50vh; background-repeat: no-repeat; background-color: hsl(185, 75%, 39%); display: flex; justify-content: center; align-items: center; width: 100%; height: 100vh; }- To center the card.
.card__body { width: inherit; height: inherit; display: flex; justify-content: center; align-items: center; }Happy coding๐งโ๐ป
1 -
- Nicolas Ramosโข 10
@RamosNico
Submitted
This is the first challenge I've ever done, felt pretty good.
Both Background's .svg are placed in order to look good at widths: 1440px and 375px through media queries, but as soon as the screen gets bigger they will move all over the place, not sure how to fix it.
Also, I can't manage to truly align the card's footer with the main body, the statistics are a bit more to the right compared with the name, although I centered all of them. I could center them manually with some right-padding, but does anyone know how to fix it without going that way?
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hello, ๐๐ป RamosNico.
Well done my man, in your first challenge ๐๐๐. Lovely job on this challenge, place properly the background image is quite tricky or at least it was for me.
I've been digging into your code. And this is how I would do it.
- I would delete the images from the Html. and places it in the Css.
body { background-color: #19a2ae; background-repeat: no-repeat; font-family: "Kumbh Sans", sans-serif; background-image: url(../images/bg-pattern-top.svg), url(../images/bg-pattern-bottom.svg); background-repeat: no-repeat, no-repeat; background-position: right 50vw bottom 50vh, left 50vw top 50vh; }Regarding the alignment of the card's footer with the main body, I see it well. They have a display: flex. justify-content: space-evenly.
I hope, it helps. Happy coding๐งโ๐ป
2 - Ednaโข 10
@edshuli
Submitted
Hello everyone, can you please give me an advice about the Html part, especially about the card bottom. I did it with the <table> element, would you do it another way ?
Thank you for your time !!
Have fun :)
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hello, ๐๐ป edshuli. Well done my friend ๐๐๐. Lovely job on this challenge, place properly the background image is quite tricky.
Just a suggestion in my humble newbie opinion. ๐
I've been checking your code, I would approach that, like so.
- I think you can avoid those media queries. Having a wrapper, wrap card, and card-bottom, and in your CSS, I would do this to center the card.
.wrapper { display: flex; flex-direction: column; justify-content: center; align-items: center; height: 100vh; }-
It would still not be fully centered, so you have to remove the margins from the card.
-
To position the background-images properly and make them responsive, I would do this on your CSS body.
background-position: right 50vw bottom 50vh, left 50vw top 50vh;- Add an alternative text to your images.
<img src="/images/image-victor.jpg" alt="victor crest">- The id global attribute defines an identifier (ID) which must be unique in the whole document. Its purpose is to identify the element when linking (using a fragment identifier), scripting, or styling (with CSS). And you have more than one with the same name.
I hope, it helps.
Happy coding๐งโ๐ป
1 - PDogukan Varisโข 540
@varisDogukan
Submitted
I don't have any experience with naming classes. Can you help me for this?
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi, do-Va.
There are only two hard problems in Computer Science: cache invalidation and naming things โ Phil Karlton
I recommend you, learn BEM too, it will make your life easier. You can check this video. You Probably Need BEM CSS in Your Life (Tutorial) or check the BEM website about naming Naming
I hope, it helps.
Happy coding๐งโ๐ป
2 - Rizky Firman Syahโข 215
@firmansyahrizky
Submitted
If you found any problems around it please tell me, I'll fix that ASAP.
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป RizkyFirman.
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐, I like the animation cards and itโs very responsive ๐ฏ.
I have been digging into your code, you have used css-grid. Cool.
You have a horizontal scroll on a small and medium screen. I would put on the body an
overflow: hiddento fix that.You got many
<div>. Just a suggestion in my humble newbie opinion. ๐-
You could have a
<main>tag involving your code. The<main>element represents the dominant content of the<body>of a document. -
Your cards could be
<article>instead of just<div>
I hope, it helps.
Happy coding๐งโ๐ป
2 -
- Harsh Porwalโข 10
@h-harsh
Submitted
i was not able to set up the background 2 circles in a responsive way, they break when screen size moves
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi, harsh.
I've been digging into your code. And this is how I would do it.
- I would give your card width, height, and a box-shadow.
.card-box { width: 353px; height: 376px; background-color: white; border-radius: 1rem; overflow: hidden; box-shadow: -30px 50px 50px #00000030; }-
I would delete the images from the Html. and places it in the Css.
-
Now place the card in the center, so in the main it would do something like this.
.main { width: 100%; height: 100vh; display: flex; justify-content: center; align-items: center; }- Finally to the body I would place the background-images.
body { background-image: url(./images/bg-pattern-top.svg), url(./images/bg-pattern-bottom.svg); background-repeat: no-repeat, no-repeat; background-position: right 52vw bottom 38vh, left 49vw top 51vh; }Hopefully, it helps.
Happy coding๐งโ๐ป
1 - Anurag Mohapatraโข 20
@mohapatraanurag
Submitted
Could setup the circles for 1440px and 375px but when checked other resolution the bottom circle moves around. Would appreciate if anyone can give feedback on this.
Working with sass for the first time and with HTML and css after a while.
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hello, ๐๐ป mohapatraanurag. Well done my friend ๐๐๐. Lovely job on this challenge, place properly the background image is quite tricky.
Just a suggestion in my humble newbie opinion. ๐
-I've been checking your code. And I would approach that, like so.
-Try to remove all unnecessary comments.
-Remove the
bg-patternfrom the Html, this way you avoid those Html issues. And in your sass file_global.scssI would place the bg images.body { background-image: url(../images/bg-pattern-top.svg), url(../images/bg-pattern-top.svg); background-repeat: no-repeat, no-repeat; background-position: right 52vw bottom 38vh, left 49vw top 51vh; }I know, there are many ways to place background properly, But that's how I solved it.
I hope, it helps.
Happy coding๐งโ๐ป
1 - Ivetโข 210
@Ivet89
Submitted
Hola :)
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hola, Ivet89. You have done a fantastic job on this challenge ๐, and itโs quite responsive ๐ฏ.
You have used bootstrap 4. why not bootstrap 5.
I have been checking your code, and you got many
<div>. Just a suggestion in my humble newbie opinion. ๐-
You could have a
<main>tag involving your code. The<main>element represents the dominant content of the<body>of a document. -
Your cards could be
<article>instead of just<div>
Happy coding๐งโ๐ป
1 -
- Gabriel Alejandroโข 310
@gam98
Submitted
Renszo Camachoโข 1,615@RenszCamacho
Posted
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐
Just a suggestion in my humble newbie opinion. ๐
It would be great some margin-bottom on the mobile version.
Happy coding๐งโ๐ป
1 - Domiโข 20
@domihustinova
Submitted
Hi everyone ๐๐ป This is my first Frontend Mentor challenge. Until now, I was mostly focused on JS and neglecting my CSS skills so I've decided to work on those a bit, too. I had also zero experience with SASS before I started this task so I guess that was the biggest challenge for me :) I would appreciate any feedback!
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi ๐๐ป domihustinova. Well done ๐๐๐. You have done a fantastic job on this challenge ๐, I noticed that you did desktop-first, and itโs quite responsive ๐ฏ.
Happy coding๐งโ๐ป
1 - yossef alatterโข 540
@yossefAlatter
Submitted
- Hello everyone
- that is a good challenge with nice functionality i also do the animation bonus
- i create my own methods to do that function and use it to make the timer and the animation take a look to the code time CompleteTimerPieces Components
- also i make it responsive as much as i can
- i will be very happy by your feedback
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป yossefAlatter. Well done my friend ๐๐๐. You have done a fantastic job on this challenge ๐
Happy coding๐งโ๐ป
2 - Filip Viktor Hanzlรญkโข 30
@FilipHanzlik
Submitted
Hello! A feedback would be highly appriciated and would help me improve๐. Also i had a problem setting shadows in hover state. Basically after aplying a hover state for all cards, only the first one works correctly. If anyone knows what i did wrong, i would really appreciate if he let me know.
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi FilipHanzlik.
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐, I like the animation background and itโs very responsive ๐ฏ.
It took me a while to find the shadow's bug. ๐
The issue is your layout and how you placed the backgrounds, what is above your cards.
So to fix that just put a
.bottom-background{ z-index: -10 }Hopefully, it helps.
Happy coding๐งโ๐ป
3 - Alper Mehmet รZdemirโข 75
@AlperMehmetOzdemir
Submitted
I couldnt get the overhaning phone image from the hero section to the feature(why choose easybank) section. If I use "overflow-x:hidden" in the body it fixes it for 1440px and up ut causes issues for lower resolutions. If i use "overflow-x:hidden" in the class itself or the direct parent class then it makes it a scrollable image. How would I fix that?
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป AlperMehmetOzdemir. Well done my friend ๐๐๐. You have done a fantastic job on this challenge ๐,I noticed that you did mobile-first, and itโs responsive ๐ฏ.
I've been digging into your code, and I reckon, that removing the
overflow: hiddenfrom.hero .container { overflow-x: hidden; }You can fix that horrible scrollbar
Hopefully, it helps.
Happy coding๐งโ๐ป
2 - Maria Urdapilletaโข 55
@mariaUrda
Submitted
Feedback will be appreciated :)
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป mariaUrda. Well done my friend ๐๐๐. You have done a fantastic job on this challenge ๐, and itโs responsive ๐ฏ.
Just I few suggestions in my humble newbie opinion. ๐
The backgrounds-images are not in place, at least not on my screen, so it looks good on all screens. I would give the body a
min-height: 100vhYour user testimonial images are slightly big, I would do something like this:
.card-bottom-picture { border-radius: 50%; width: 3rem; height: 3rem;Hopefully, it helps.
Happy coding๐งโ๐ป
2 - Eric A Hodgeโข 10
@ehodg
Submitted
I thought I did a good job but I feel there are much easier ways to style than what I did. Any feedback would be great
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi, Eric. Well done.
I have been checking your code and I have seen some issues.
The first suggestion is that you do mobile-first. It's easier.
Your layout looks weird. you have the footer in the middle of the screen. I think you can place all the items, just with flex-box. Although I have seen that you have used absolute position.
Your images are broken, it is because you have set the path wrong, and for good practices, it would be great if you don't leave the
alt=""empty at least in these cases.- The alt attribute holds a text description of the image, which isn't mandatory but is incredibly useful for accessibility โ screen readers read this description out to their users so they know what the image means. Alt text is also displayed on the page if the image can't be loaded for some reason: for example, network errors, content blocking, or linkrot.
This how Id solve it.
<img src="./images/image-colton.jpg" alt="Colton Smith" />Hopefully, it helps.
Happy coding, mate ๐งโ๐ป
2 - luccaslopes88โข 130
@luccaslopes88
Submitted
Totally responsive, built with HTML and CSS. I feel like this is kinda messy.. Feel free to slam me with feedback!. Cheers everyone!
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป luccaslopes88.
Well done my friend ๐๐๐. You have done a fantastic job on this challenge ๐, I noticed that you did mobile-first, and itโs very responsive ๐ฏ.
Happy coding๐งโ๐ป
2 - Krishna Tandonโข 10
@krishnatandon1208
Submitted
Hi, Please provide the feedback for the solution. Whether it is in proper structure or not, how can I make it better if the existing solution is correct.
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hello mate.
You haven't placed the background-color and the background images, I just can see one. So I have gotten into your code and I did this to fix it. Check if that works for you too.
.master-card-block { background-color: hsl(185, 75%, 39%); background-image: url("bg-pattern-top.svg"), url(./bg-pattern-bottom.svg); background-repeat: no-repeat; background-position: calc(100% - 50vw) calc(100% - 50vh), calc(0% + 50vw) calc(0% + 50vh); height: 100vh; }Greetings.
3 - Benzโข 60
@benzrire
Submitted
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป benzrire.
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐, I like the way how you sorted the backgrounds images out and itโs very responsive ๐ฏ.
๐Nice solution!!!! Keep coding๐งโ๐ป
2 - kathlenetajoneraโข 970
@kathlenetajonera
Submitted
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป ktj13.
Good job mate ๐๐๐. You have done a fantastic job on this challenge ๐, I like the way how you approach this challenge and itโs very responsive ๐ฏ.
Nice solution!!!! ๐ Keep coding๐งโ๐ป
2 - M.S.Madhan Kumarโข 10
@madhankumarms
Submitted
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hiya ๐๐ป madhankumarms. Well done my friend ๐๐๐. You have done a fantastic job on this challenge ๐, and itโs quite responsive ๐ฏ.
Happy coding๐งโ๐ป
1 - Abhikโข 4,840
@abhik-b
Submitted
Hello Frontend Mentors ๐
I tried my best on this challenge to make it perfect, I have used nextjs with typescript and this was a kind of practice project for me using useReducers and useContext hook !
So please provide any feedback , tips, suggestions related to my code or solution ๐, I'll highly appreciate your help
I actually recorded a youtube video while solving this , so if you are stuck with this challenge , you can use it as a reference๐ค
Renszo Camachoโข 1,615@RenszCamacho
Posted
My man. That's sleek. Can't say anymore. I do like it. ๐๐๐. I'm gonna check your Youtube channel right now.
Greetings.
Marked as helpful
2 - Praveen Kumarโข 30
@praveen-kumar-m
Submitted
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hello, ๐๐ป praveen. Well done my friend ๐๐๐. Lovely job on this challenge, place properly the background image is quite tricky.
Just I few suggestions in my humble newbie opinion. ๐
-I've been checking your code. And I would approach that, like so
.container { background-image: url(/images/bg-pattern-top.svg), url(/images/bg-pattern-bottom.svg); background-repeat: no-repeat, no-repeat; background-position: right 52vw bottom 38vh, left 49vw top 51vh; }
I would place the background-images in your 'wrapper' or the way that you do it, I'll adding some media queries to fix the position in every breakpoint.
I hope it helps.
Happy Coding! ๐งโ๐ป
3 - Birca Andreeaโข 20
@bircaandreeadev
Submitted
If you'd like to give me any suggestions on how to improve my frontend knowledge, I would appreciate it!
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi, bircaandreeadev. This challenge is a little bit tricky, or at least was for me.
Just some issues.
When I open all the tabs of the accordion. the text overflows, and the images are stretched. Try to fix that.
That is how I did it.
I would give your 'ul' a fixed height, and overflow:auto, in this way the content does not overflow. But you will get an ugly scroll.
ul { padding: 0; position: relative; overflow: auto; height: 300px; }This is to hide the scrollbar.
ul::-webkit-scrollbar { display: none; }Hopefully, it helps.
Happy coding๐งโ๐ป
2 - Bruno Mazzaโข 90
@DownTheMatrix
Submitted
In order to move the illustration image as per the design (displaced from its original position and overlapping the container), I used the "transform: translate()" property. Now, I initially tried with "position: absolute", but I wasn't too happy with the result. I would love to hear what you guys think :)
Renszo Camachoโข 1,615@RenszCamacho
Posted
Hi DownTheMatrix.
You have done a fantastic job on this challenge ๐. and itโs responsive ๐ฏ.
I just miss the background pattern. Overall looks pretty good. ๐
Happy coding๐งโ๐ป
2
