Preview-card component ( Mobile / Desktop layout )

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  Hi

  Here is feedback I've made in browser devtools. Go through each item in turn and look things up as and when you need to.

  I hope this helps you :)

  
  /* Element | https://handyyweb.github.io/Preview-card/ */
  
  body > main:nth-child(1) {
  /* word-wrap: break-word; */
  }
  
  /* index.css | https://handyyweb.github.io/Preview-card/index.css */
  
  @media only screen and (min-width: 600px) {
  main {
  /* max-width: 150%; */
  /* min-height: 50%; */
  max-width: 45rem;
  note: media queries should always be defined in rem or em;
  display: flex;
  }
  /* div.image { */
  .image {
  /* float: left; */
  /* padding: 0; */
  }
  /* div.content { */
  .content {
  /* float: right; */
  /* padding: 2rem 1rem 0rem 1rem; */
  padding: 2rem;
  filter: ;
  display: flex;
  flex-direction: column;
  }
  div > p {
  /* padding-top: 2rem; */
  margin-top: 2rem;
  }
  }
  
  main {
  /* min-height: 35rem; */
  }
  
  div {
  /* max-width: 20rem; */
  }
  
  /* div.content { */
  .content {
  }
  
  button {
  /* margin-top: 1rem; */
  /* bottom: 4px; */
  margin-top: 2rem;
  display: flex;
  align-items: center;
  justify-content: center;
  gap: 1ch;
  cursor: pointer;
  transition: all 0.3s ease-out;
  note: Use the colors from the style guide. I think you may be using a hover color here;
  }
  
  div > small {
  /* padding-top: 5px; */
  /* letter-spacing: 6px; */
  letter-spacing: 0.4em;
  note: dont use complex css selectors like this unless you absolutely have to. Use single class selectors. Keep CSS specificity low!;
  note: Small is not a meaningful element on its own. It should be inside a paragraph;
  note: Letter spacing MUST NEVER be in px!;
  note: define font size explicitly in rem;
  }
  
  body {
  note: I don't think fonts are being correctly applied in this solution. Check them';
  }
  
  div > h1 {
  /* padding-top: 1rem; */
  margin-top: 1rem;
  note: I think you are confused between padding and margin. Look up the differences. Margin is for _external_ spacing between elements, padding is for _internal_ spacing inside elements (like boxes);
  }
  
  div > p {
  /* padding-top: 1rem; */
  /* padding-bottom: 1rem; */
  /* line-height: 26px; */
  margin-top: 1rem;
  line-height: 1.6;
  note: Line height must NEVER be in px;
  note: generally on child elements you'll only want margin to go in one direction (top in this case)';
  }
  
  span.price {
  /* margin-top: 3rem; */
  margin-top: 2rem;
  note: You MUST use meaningful elements on this. Both prices should be wrapped in a paragraph set to display flex;
  note: Dont use key words for font weight. Use the exact value eg 700 or 800 or whatever the weight is you've downloaded';
  display: block;
  }
  
  span {
  /* padding: 1em; */
  /* width: 100%; */
  note: don't use inline styles';
  }
  
  del {
  note: Screenreaders need additional visually-hidden information to identify this as the old price;
  note: use colors as defined in the style guide;
  margin-bottom: auto;
  }
  
  svg {
  note: this svg needs 'aria-hidden="true" focusable="false"' added to it so the svg is treated as decorative by all screenreaders;
  }
  
  .image img {
  height: 100%;
  note: place a class on what you want to style;
  object-fit: cover;
  }
  
  

  I've done a post about the html on this challenge, which may help you when I mention meaningful elements and informing screenreaders about the prices. There are other posts on there about font property units etc if you want to look around

  Marked as helpful

  0

  ---

  

  Handyyweb•120

  @HandyyWebPosted over 1 year ago

  @grace-snow

  Oh ! Thank you for the detailed feedback. Now I see that there is still a lot to do haha ! I will remember all the notes you give me for future projects. Have a great day/night !

  1
• 

  Rostyslav Nazarenko•560

  @rostyslav-nazarenkoPosted almost 2 years ago

  I'm only a beginner but played with your code for a bit and I have a few suggestions.

  • add box-sizing: border-box with this you'll fix divs splitting in different conners.

  html {
    box-sizing: border-box;
  }
  
  *,
  *::before,
  *::after {
    box-sizing: inherit;
  }
  

  • I would increase the breakpoint from 24rem to something around 600px so it wouldn't be so compressed

  Marked as helpful

  0

  ---

  

  Handyyweb•120

  @HandyyWebPosted almost 2 years ago

  I tried what you said and it works, thank you ! Just, I have deleted the *::before because they was wome struggles with the <button> tag like this :

  *{
      margin:0px;
      padding:0px;
      box-sizing: border-box;
  }
  
  *::after {
    box-sizing: inherit;
  }
  
  

  Moreover, the 600px breakpoint was a very good idea, fix most of the bugs, thank you and have a great day ! I will add your name, to thank you, on my README.md file.

  1
• 

  Sameer Md•540

  @Sameermd16Posted almost 2 years ago

  Card component is not responsive between 375px to 706px. Try making making responsive by adding media queries in the format @media only screen and (min-width: 376px) and (max-width: 706px) { "here comes css" }

  Marked as helpful

  0

  ---

  

  Handyyweb•120

  @HandyyWebPosted almost 2 years ago

  @Sameermd16

  Okay, i will try that thank you ! However, I don't know what is the problem here, which is making it not responsive. I tried to fix the padding of the div content but nothing had changed.

  0