Grace

Try not to focus on speed when completing challenges. Focus on quality including testing with keyboard, zoom and screenreaders.

This is a really good attempt, just a few issues I can see straight away

1. Make sure the component has some space around it so it never touches screen edges. It is hitting the top of the screen on some phones. You can do this with margin on the card or padding on the body in this case

2. Very important not to limit the body height to 100vh. That cuts off content on some devices. Use min-height: 100vh not height

3. This would not use a button element. Unless you're expecting something else to happen on click? I would expect it to be an anchor tag.

4. Screenreaders aren't told when strikethrough effects are applied on text. So you need to do a bit more to make sure those users know what is the old price vs the new one. As it stands they would hear both prices, which is very confusing. There are lots of ways to handle strikethrough if you look them up online.

5. I wouldn't expect to see width as such a small percentage and min-width on this. Instead, let it be 100% wide (as per the default) and set a max-width instead.

Good luck

Hi

This seems a bit rushed. It''s got explicit heights and widths meaning this will break for any users who resize their font/zoom settings. Test what I mean by setting your browser to 'zoom text only' then zoom in on desktop. You will see content spill out of the card.

There is rarely a good reason to limit height, which is why we recommend min-height instead.

I also recommend against using input type button. That's a deprecated element from before the button element existed! And anyway, looking at this design, I don't think it would be a button at all, it would be an anchor tag.

Really take your time and think through the html carefully.

Last thing - the image gets distorted in some views. Check out the object-fit property!

Hi

There is loads on here that doesn't need to be on here. If you worked mobile first, your code would be a lot shorter. You also have a lot of duplication across each card. They have loads of shared styles so can have one class that does all of that. Then they each need one extra class, or you can use a selector like nth child to target each one and override the border color, and then the grid position on desktop.

Here is what I just changed in browser. Most of it was commenting out unnecessary duplication in media queries

/* main.css | https://usancode.github.io/four-card-feature-section-master/four-card-feature-section-master/main.css */

.grid-container {
  /* width: 58rem; */
  /* height: 30rem; */
  /* padding-top: 2%; */
  /* padding-bottom: 2%; */
  max-width: 58rem;
}

@media screen and (max-width: 820px) {
  .grid-container {
    /* width: 52rem; */
    /* height: 30rem; */
    /* display: grid; */
    /* grid-template-rows: 1fr 1fr 1fr 1fr; */
    /* grid-template-columns: 1fr 1fr 1fr 1fr 1fr 1fr; */
    /* row-gap: 1.5rem; */
    /* column-gap: 1.5rem; */
    /* margin: auto; */
    /* padding-top: 2%; */
    /* padding-bottom: 2%; */
    /* justify-content: center; */
    grid-template-columns: auto;
  }
  body {
    /* background-color: hsl(0, 0%, 98%); */
    /* margin-right: 4%; */
    /* margin-left: 3%; */
    /* margin-top: 5%; */
    /* margin-bottom: 5%; */
    /* max-width: 1440px; */
    /* width: 100%; */
    /* justify-content: center; */
  }
}

body {
  /* margin-right: auto; */
  /* margin-left: auto; */
  /* max-width: 1440px; */
  /* width: 100%; */
  /* justify-content: center; */
  margin: 0;
  padding: 1rem;
}

.heading {
  /* line-height: 0.4rem; */
  /* text-align: justify; */
  /* text-justify: distribute-all-lines; */
  /* justify-items: center; */
  /* margin: auto; */
  /* padding-bottom: 0.5rem; */
  /* grid-column-start: 1; */
  /* grid-column-end: 2; */
  /* grid-row-start: 1; */
  /* grid-row-end: 2; */
  line-height: 1.3;
  note: line height should be unitless;
}

.mainBlock {
  /* display: grid; */
  /* grid-template-columns: 100%; */
  /* grid-template-rows: 30% 30%; */
  note: why all these divs?;
  margin-bottom: 1rem;
}

.mainContainer {
  /* width: 75%; */
  /* margin: auto; */
  /* justify-content: center; */
  margin: 0 auto 2rem auto;
  text-align: center;
}

.motto {
  /* width: 50%; */
  max-width: 50ch;
}

@media screen and (max-width: 540px) {
  .mainBlock {
    /* display: grid; */
    /* grid-template-columns: 100%; */
    /* grid-template-rows: 30% 30%; */
  }
  .mainContainer {
    /* width: 100%; */
    /* margin: auto; */
    /* justify-content: center; */
  }
  .heading {
    /* line-height: 0.4rem; */
    /* text-align: justify; */
    /* text-justify: distribute-all-lines; */
    /* justify-items: center; */
    /* margin: auto; */
    /* padding-bottom: 0.5rem; */
    /* grid-column-start: 1; */
    /* grid-column-end: 2; */
    /* grid-row-start: 1; */
    /* grid-row-end: 2; */
  }
  .motto {
    /* text-align: justify; */
    /* justify-items: center; */
    /* width: 15rem; */
    /* height: 3rem; */
    /* margin: auto; */
    /* padding-bottom: 0.3rem; */
    /* font-size: 14px; */
  }
  .grid-container {
    /* display: grid; */
    /* grid-template-columns: 16rem; */
    /* grid-template-rows: 13rem 13rem 13rem 13rem; */
    /* width: 20rem; */
    /* height: 56rem; */
    /* row-gap: 1rem; */
    /* justify-content: center; */
    /* margin: auto; */
    /* padding-top: 2%; */
    /* padding-bottom: 2%; */
  }
  .firstBlock {
    /* border-top-style: outset; */
    /* border-top-color: hsl(180, 62%, 55%); */
    /* background-color: hsl(0, 0%, 98%); */
    /* border-radius: 0.2rem; */
    /* box-shadow: 0.1rem 0.5rem 0.5rem hsl(229, 6%, 66%); */
    /* padding-bottom: 1rem; */
    /* padding-left: 2rem; */
    /* padding-right: 2rem; */
  }
  .secondBlock {
    /* border-top-style: outset; */
    /* border-top-color: hsl(0, 78%, 62%); */
    /* background-color: hsl(0, 0%, 98%); */
    /* border-radius: 0.2rem; */
    /* box-shadow: 0.1rem 0.5rem 0.5rem hsl(229, 6%, 66%); */
    /* padding-bottom: 1rem; */
    /* padding-left: 2rem; */
    /* padding-right: 2rem; */
  }
  body {
    /* width: 100%; */
    /* background-color: hsl(0, 0%, 98%); */
    /* justify-content: center; */
  }
}

// Usually a css reset at the start will remove browser styles like margin on headings 
h1, h2 {
  margin: 0;
}

Hi

I think you're getting confused when it comes to layouts. A few big issues you need to understand better

1. Setting max-width in % means you lose all control over the widths. You don't know what 70% equates to as it all depends on a user's screen size. Instead, set max-width in rem for layouts. EG:

/* styles.css | https://yousra10.github.io/four-card-future-section-master/styles.css */

h1, .top-container {
  /* max-width: 70%; */
}

p {
  /* max-width: 60%; */
  max-width: 65ch;
}

1. Your grid doesn't match at all what is needed here. You are setting sooooo many columns and rows (I'll add screenshot in slack). You should only be setting exactly what you need, then laying items out on that. In this case, that's (for desktop) I think it would be 3 columns and 4 rows. You may find using grid-template areas helps a lot to visualise where you're placing content instead of all the row and column number properties

2. You are using margin and padding in strange ways in the card. Create one card class to set the base properties on all of them like border-radius, padding, shadow, background color, top border. Then each card only needs its grid area setting and the border-color. Inside the cards, those elements only need vertical margin (not margins all over or padding) and the image can be right-aligned in various ways, but I wouldn't recommend absolute positioning. It would be easier kept in the document flow and adjusted with left margin auto or flex properties

Other that that only some minor adjustments to recommend:

• It should be one h1 not 2. You can use strong tag set to display block to create the line break in it
• Never jump heading levels. They must go in order (essential)

Hey, nice solution.

This is really good, but I think you need to work a little on consistency in the way you're using clamp to set widths, and work on html knowledge overall. For example, if you were to turn off all styles, would the html structure make sense on its own and be formatted how you would expect. That's a good way to check everything makes sense (i.e. would I expect a list there, would I expect that to be a button or a link, etc)

Here are notes I made in browser, I hope they are helpful

.hero__content.container {
  note: again, too large a minimum. This is wider than my phone screen;
}

.btn {
  note: why is this a button element?;
}

.features.grid {
  note: sections are unnecessary for each box in here. They act exactly like divs when unlabelled anyway;
  note: consider an sr-only heading of 'Features' followed by each feature having a h3;
}

.productive .content {
  /* margin-left: 1.5em; */
}

.productive .title {
  text-align: left;
}

.productive.flex {
  note: changes within this due to alignment issues on mobile;
}

.testimonials.flex {
  note: similar to previous - consider adding an sr-only h2 like 'testimonials;;
}

.testimonials .testimonial {
  note: consider using figure and figcaption. Blockquote can go inside the figure. Totally optionally idea;
}

.access .title {
  note: why is this a h3?;
}

.access form input[type="email"] {
  note: label should say what the field is, not be written as instructional text;
}

.access form .error {
  note: this should be in an element that has an aria-live attribute on it. That element must not be display none.;
  note: that error element wrapper will also need a unique id, and the input should have aria-describedby the ID;
}

.bg-footer {
  note: why is footer section a different width to content above on mobile? They should all be consistent;
}

footer nav > ul {
  note: Lists should definitely not be used for address email and telephone ! A list is not a list when it only has one item inside;
  note: only the links About us, jobs etc right through to privacy should be a list inside the nav. (one list not two) All of these should have anchor tags;
}

footer .social-list {
  note: Not part of the site nav so should not be in a nav element;
}

footer .social-list a {
  note: consider making these social links open in a new window. If you do that remember to include rel="noopener" as well for security reasons;
}

footer a {
  note: if a link, the Fylo logo link alt should say the link destination as well eg Fylo - Home;
}

footer nav {
  note: the first half of this is not nav content. Address telephone and email links do not belong in a nav. You could wrap the whole of the first half of the footer (including logo) in an address element if you wanted. That is intended to hold site author information.;
}