Need Help improving my code
I'm trying to recreate this challenge, but I'm stuck on how I should have laid out my elements. I used grid but I'm not sure if i should include the testimonies in the bottom in the same grid or make that its own thing. And how I should I moves the ratings. I used position relative for the ratings but when the browser shrinks they over flow so I just wanted some advice on what I could do better and make my code more efficient.
4 Replies
The stars aren't loading here but they load in VS code
Going to address a lot more than the questions you asked, but I will get to those too 😄
Using BEM
Looks like you're using BEM, but going a little too heavy handed on it.
If you are using BEM, you shouldn't have nested blocks, like
.grid__item-1__title. It just becomes really cumbersome, and you'll end up with crazy class names...
The __ means that thing has to live in whatever is before it. In this case, .grid is a layout, but the item-1 could be taken out of that context and work fine.
I'd also avoid things like item-1 and go for more meaningful names. That's the hard bit of course, but I'd probably do something like this:
I don't love the .about name, but it's better, and then you could have .about__title and .about__desc.
It also raises the question of whether or not you even need a class on it at all. I mean, this would be fine, and then you don't even need to worry about it:
You'll only have one h1 on a page anyway, so it gets it's styling that way, and the paragraph that follows looks like a normal paragraph... the only style you've put specifically on it right now is a max-width: 45ch... that could go on all your paragraphs, at least for this layout 🤷. There are other ways you can select it to, like a h1 + p { ... }.
Anyway, just some food for thought 🙂
Can't put divs inside of paragraphs
Yyou can't nest <div> inside of <p>, which you did for the content section in the reviews. If you look at it inside your dev tools, you'll see that the browser actually closes your <p class="content"> before it opens the image container part. You can only include phrasing content inside of paragraphs, which is generally things that are inline by default, like spans, em, strong, etc.
The layout
As for one big grid, or two separate parts, with how you created the two columns at the top, which works, I'd just put in a second part at the bottom to create the three columns.
There are ways you could do this with one larger grid, but it'll probably be easier to have two parts.
The star ratings
As for the star rating, instead of using positioning, I'd just use margin-left: auto; margin-right: auto on the middle one to center it, and only a margin-left: auto on the last one, to push it all the way to the right. With the broken images, I set a max-width: 80% on them for this to work, but you probably don't need it.
Speaking of the stars
I'd leave the alt="" blank on the image of the stars.
Alt text is used to give more context if an image is missing, but we already have all of that context because of the "Rated 5 stars in Reviews" text that is right after them.
Alt text is read by screen readers, so imagine a user with poor vision, or who's blind, who's using a screen reader... It'll read out "image, star... image, star... image, star... image, star... image, star" and then "Rated 5 stars in Reviews". That would suck 😅.
alt text is incredibly important, but you want to use it to describe an image, assuming a person can't see the image, and because that image is adding context or value to the experience. If the image is purely decorational, as it is with the stars, you want to keep the alt="", but leave it blank. That tells screen readers it's decorational.@Kevin Thank you for the advice! I was a bit late to this because I haven't been on discord but I appreciate the help it means a lot and will definitely apply everything here.