
Design comparison
Solution retrospective
I am proud to keep improving my skills and successfully complete another challenge.
What challenges did you encounter, and how did you overcome them?I'm not entirely sure if I'm using the BEM method correctly to name classes, but I believe I'm doing fine for now.
What specific areas of your project would you like help with?I am open to feedback.
Community feedback
- P@rickydoddPosted 13 days ago
Hello! I wanted to offer some critiques, which I hope will be valuable to you. It can be intimidating to have a stranger on the Internet offer their opinion on matters, so I hope you take this well.
Before we get to the critiques, I wanted to say: well done! Practice makes perfect, and I am always glad to see people practicing website development (or, really, programming of all kinds).
Now, on to the actual critiques.
What you did well:
- A good attempt at using the BEM methodology, but there is a lot that can be improved.
- Using the HSL function and modern approaches to using color.
- The solution is, visually, similar to the design.
What could use improvements, in my opinion:
- Naming custom properties (i.e., "variables")
Use names that refer to the color, not what the color is used for. You did this for most of the variables, for example
Brightorange
, but you later changed this convention with names such asTransparentwhiteparagraphs
. To keep consistent, you should change custom property names such asTransparentwhiteparagraphs
toTransparentwhite
, as, when a design evolves, you may find that you need this color in places that are not strictly paragraphs.You also want to separate the words in your custom property names more clearly, as they are difficult to read. For example,
Transparentwhite
could betransparent-white
ortransparentWhite
, so that it is easier to discern the separate words that form the custom property name.The following are the custom properties as you have defined them:
:root { --Brightorange: hsl(31, 77%, 52%); --Darkcyan: hsl(184, 100%, 22%); --Verydarkcyan: hsl(179, 100%, 13%); --Transparentwhiteparagraphs: hsla(0, 0%, 100%, 0.75); --Verylightgraybackgroundheadingsbuttons: hsl(0, 0%, 95%); }
And this is what I suggest instead:
:root { --bright-orange: hsl(31, 77%, 52%); --dark-cyan: hsl(184, 100%, 22%); --very-dark-cyan: hsl(179, 100%, 13%); --semi-transparent-white: hsla(0, 0%, 100%, 0.75); --very-light-gray: hsl(0, 0%, 95%); }
- Improve the use of the BEM methodology
Your use of BEM is not very reusable, and the names are tied with the type of content. For example, if we wanted to add another type of cars, "convertibles," then we would need to create another class (
card__convertible
), and copy and paste the majority of the declarations of an existing class (card__sedans
,card__suvs
,card__luxury
) into this new class. This is not ideal, as it decreases reusability.Here is the code from your GitHub repository, but extended to add the
card__convertible
class, to help illustrate how many declarations need to be copied and pasted from one ruleset to this new ruleset when adding another type of car..card { width: 770px; height: 420px; display: flex; border-radius: 10px; } .card__sedans { display: flex; flex-direction: column; background-color: var(--Brightorange); border-radius: 10px 0px 0px 10px; padding: 40px; gap: 5px; } .card__suvs { display: flex; flex-direction: column; background-color: var(--Darkcyan); padding: 40px; gap: 5px; } .card__convertible { display: flex; flex-direction: column; background-color: var(--Anothercolor); padding: 40px; gap: 5px; } .card__luxury { display: flex; flex-direction: column; background-color: var(--Verydarkcyan); border-radius: 0px 10px 10px 0px; padding: 40px; gap: 5px; }
And if you want this to be the first or the last card, then you will even need to change some of the declarations in some of these existing classes. Instead, I suggest creating a more generic class. I suggest renaming the outer element's class from
card
tocards
(denoting it contains multiple cards), and condensing the classescard__sedan
, etc, to a class calledcard
. So the previous CSS becomes:.cards { width: 770px; /* I also changed height to min-height, so that the content can still grow vertically when, and if, it needs to. */ min-height: 420px; display: flex; border-radius: 10px; } .cards:first-child { border-radius: 10px 0 0 10px; } .cards:last-child { border-radius: 0 10px 10px 0; } .card { display: flex; flex-direction: column; padding: 40px; gap: 5px; } .card--bg-bright-orange { background-color: var(--Brightorange); } .card--bg-dark-cyan { background-color: var(--Darkcyan); } .card--bg-very-dark-cyan { background-color: var(--Verydarkcyan); }
And then the HTML will look something like the following:
... <div class="cards"> <div class="card card--bg-bright-orange"> <img class="card__icon" src="./images/icon-sedans.svg" alt="sedans icon"> <h2 class="card__title">Sedans</h2> <p class="card__description">Choose a sedan for its affordability and excellent fuel economy. Ideal for cruising in the city or on your next road trip.</p> <button class="card__button">Learn More</button> </div> <div class="card card--bg-dark-cyan"> <img class="card__icon" src="./images/icon-suvs.svg" alt="suvs icon"> <h2 class="card__title">SUVs</h2> <p class="card__description">Take an SUV for its spacious interior, power, and versatility. Perfect for your next family vacation and off-road adventures.</p> <button class="card__button">Learn More</button> </div> <div class="card card--bg-very-dark-cyan"> <img class="card__icon" src="./images/icon-luxury.svg" alt="luxury icon"> <h2 class="card__title">Luxury</h2> <p class="card__description">Cruise in the best car brands without the bloated prices. Enjoy the enhanced comfort of a luxury rental and arrive in style.</p> <button class="card__button">Learn More</button> </div> </div> ...
Note, you will need to make sure the changes reflect through the rest of your CSS, including inside the media query and hover states, etc.
- The implementation does not pass Success Criterion 1.4.10: Reflow
At screen dimensions 320px (width) by 256px (height), a horizontal scrolling is possible, which is not necessary for the functionality of the website. As such, the implementation does not pass what I consider as an important accessibility criterion.
- The buttons are not obviously interactable with on desktop.
This is easily solved with the
cursor: pointer
declaration on the buttons.So the following
.card__button { margin-top: 60px; width: 120px; height: 100px; border-radius: 20px; border: none; background-color: var(--Verylightgraybackgroundheadingsbuttons); font-weight: 900; }
becomes
.card__button { margin-top: 60px; width: 120px; height: 100px; border-radius: 20px; border: none; background-color: var(--Verylightgraybackgroundheadingsbuttons); font-weight: 900; /* The line below is the only change to resolve this issue. */ cursor: pointer; }
Hope you find some, or all, of this feedback helpful in your journey!
Marked as helpful0@mlopezlPosted 12 days ago@rickydodd Hi Ricky, I really appreciate your feedback. I will work on it. 😀
1 - @mohamedqabbariPosted 13 days ago
excellent work, also it's featured to Use BEM method.
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord