Order Summary Component

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted almost 3 years ago

  Hello

  Most of the problems with this solution are caused by the following code:

  width: 25%;
  height: 86vh;
  position: relative;
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
  

  This is not a good approach to take for layout or centering. Instead, use max-width in rem (or px for now) on the card, don't give it a height at all and use either css grid or flexbox to center it on the page.

  Marked as helpful

  1
• 

  Katherin Meganis Phang•70

  @kmeganizPosted almost 3 years ago

  Hi, Dan, good job for finishing your challenge, that quiet a step forward. I have a few things to take note:

  1. at your Readme file, appreciate if you change the word and delete unnecessaries.
  2. problem with your accessibility, but I am not sure how it look like at mobile, only can help you with this code source below.

  this is ur code below which has problem:

  <div class="attribution"> Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>. Coded by <a href="#">Your Name Here</a>. </div>

  (explanation from google) Landmarks provide a way to organize the various types of content on a page for users of assistive technologies. When content is not contained in a landmark, it will be unreachable using landmark navigation, which is an important feature provided by assistive technologies such as screen readers. Read more here: https://fae.disability.illinois.edu/rulesets/LANDMARK_2/

  The most important landmark roles are main and navigation, as nearly every page will include at least those regions.

  You can put

  <main> <div> </div> </main>

  If you find my feedback helpful please mark as helpful. Thank you. Continue do the hard work. You can do it! You are awesome!

  Marked as helpful

  1
• 

  T

  Grace•31,270

  @grace-snowPosted almost 3 years ago

  Overall, you need to readdress how you're using padding and margin, consider accessibility and text size more and stop nesting css selectors. Here's what I just changed in browser. Go though bit by bit making the changes in dev tools and you should start to see why I'm making these changes:

  .container {
    /* width: 100%; */
    /* height: 100vh; */
    min-height: 100vh;
    display: grid;
    place-content: center;
    padding: 1rem;
  }
  
  .container .card {
    /* width: 25%; */
    /* height: 86vh; */
    /* position: relative; */
    /* top: 50%; */
    /* left: 50%; */
    /* transform: translate(-50%, -50%); */
    max-width: 22rem;
  }
  
  .container .card .content {
    note: Don't nest css selectors! Use single class selectors to keep specificity low;;
  }
  
  .container .card .content p {
    /* margin-top: 1.5rem; */
    /* padding-left: 2.5rem; */
    margin-top: 1rem;
  }
  
  .container .card .content h1 {
    /* margin-top: 1.5rem; */
    margin-top: 0;
  }
  
  .container .card .content .plan {
    /* margin: 1rem 2rem 1rem 2rem; */
    /* padding: 1rem 2rem 1rem 2rem; */
    /* grid-template-columns: repeat(4, 1fr); */
    margin-top: 1rem;
    padding: 1rem;
    grid-template-columns: repeat(3, auto);
    gap: 1rem;
  }
  
  .btn {
    /* margin-bottom: 1rem; */
    /* padding: 0.9rem 5.5rem 0.9rem 5.5rem; */
    /* box-shadow: 0 14px 10px -2px rgba(70, 32, 204, 0.24); */
    /* font-size: 0.7rem; */
    margin-top: 1rem;
    padding: 1em;
    box-shadow: 0 0.75rem 0.5rem -0.375rem rgba(70, 32, 204, 0.24);
    font-size: 0.9375rem;
    note: should be an anchor tag;
    width: 100%;
  }
  
  .container .card .content .plan .box1 {
    /* padding-right: 1rem; */
  }
  
  .container .card .content .plan .box3 {
    /* padding: 0.5rem 0 0 3rem; */
    display: flex;
    align-items: center;
    justify-content: flex-end;
  }
  
  .container .card .content .plan .box2 li:nth-child(1) {
    /* position: absolute; */
    /* padding-bottom: 0.5rem; */
  }
  
  .container .card .content .plan .box2 li {
    /* padding-right: 1rem; */
  }
  
  .container .card .content .plan .box2 li:nth-child(2) {
    /* padding-top: 1.5rem; */
  }
  
  .container .card .content a {
    /* font-size: 0.7rem; */
    font-size: 0.9375rem;
    display: inline-block;
  }
  
  body {
    overflow-x: hidden;
  }
  
  .hero {
    display: block;
    object-fit: cover;
    max-width: 100%;
  }
  
  .new-text-content-wrapper {
    padding: 2rem;
    note: smaller padding on mobile;
  }
  
  .box3 a {
    note: should be a button;
  }
  
  .box2 ul {
    note: should NOT be a ul;
  }
  
  .box2 {
    text-align: left;
  }
  
  .plan > * {
    display: flex;
    align-items: center;
  }
  
  .cancel-link {
    margin-top: 1rem;
  }
  
  

  Marked as helpful

  0

  ---

  

  Codex•160

  @codein-looneyPosted almost 3 years ago

  @grace-snow Thank you so much for taking the trouble to look and give me suggestions means a lot to me, appreciated 🙌

  0
• 

  Codex•160

  @codein-looneyPosted almost 3 years ago

  Thank you very much for your observations/suggestions. appreciated :)

  0