My version of 3 column preview card component

Community feedback

• 

  P

  Ricky Dodd•40

  @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 as Transparentwhiteparagraphs. To keep consistent, you should change custom property names such as Transparentwhiteparagraphs to Transparentwhite, 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 be transparent-white or transparentWhite, 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 to cards (denoting it contains multiple cards), and condensing the classes card__sedan, etc, to a class called card. 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 helpful

  0

  ---

  

  mlopezl•100

  @mlopezlPosted 13 days ago

  @rickydodd Hi Ricky, I really appreciate your feedback. I will work on it. 😀

  1
• 

  mohammed qabbari•150

  @mohamedqabbariPosted 13 days ago

  excellent work, also it's featured to Use BEM method.

  0