Skip to content
OverviewSolutions
20
Comments
6
Davis
@davislocs

All comments

  • Elijah•320
    @taco-nekoSubmitted over 1 year ago

    I cannot for the life of me get the image on the thank you state to load. I'm using a template literal to generate the HTML for it, and it works locally, so I don't know what the issue is at all.

    I really struggle with refactoring my code. I wanted to make the site as easy to navigate with keyboard as possible, but it made it very difficult to code, and so it's kind of a mess. If anyone has advice on refactoring I would really appreciate it.

    Interactive Rating Component using HTML, CSS (Sass) and Javascript

    1
    Davis•330
    @davislocsPosted over 1 year ago

    Hey. Im not 100% sure about this, but try uncomment this part of HTML code and image should show up.

    <div class="container thanks-state">
   <img class="thanks-image" src="/images/illustration-thank-you.svg">
   <p class="selected">You selected Add rating here out of 5</p>
   <h1>Thank you!</h1>
   <p>
     We appreciate you taking the time to give a rating. If you ever need more support,
     don't hesitate to get in touch!
   </p>
 </div>

    If you found this helpful mark it as such :)

    0
  • Davis•330
    @davislocsPosted over 1 year ago

    Hello! Job well done. I only have a couple of suggestions for improvements.

    • For better accessibility, it is better to use tags like <main> and <section>.
    • instead of <div class="wrapper"> use <main class="wrapper">. And for every box is better to use <section class="box box1"> instead of <div class="box box1">.
    • <div class="container"> is not needed here. To make code cleaner, you can center your <main class="wrapper"> by using justify-items: center on <body> selector. And also move max-width: 960px property on <main class="wrapper">. So you can totally delete <div class="container">.
    • I also suggest using rem units instead px.

    Watch this video about CSS units.

    I hope this is helpful. Happy coding! :)

    Marked as helpful
    0
  • Davis•330
    @davislocsPosted over 1 year ago

    Hey whats up!

    I have a few suggestions for you to improve your code:

    • Dont use <div> for everything. There are many other tags which you can use for better accessibility. For example use <main class="container"> instead of <div class="container"> and <section class="tittle"> instead <div class="tittle">. Instead of <div class="attribution"> use <footer class="attribution">.

    • Its better to use rem units instead of px.

    Whatch this video about css units.

    If you found this helpful please mark it as such :)

    Happy coding!

    Marked as helpful
    0
  • Shubhooooo•20
    @ShubhoooooSubmitted almost 2 years ago

    I am a beginner so it is hard for me to make responsive pages. I am still learning.Please tell if there are any suggestions

    Flexbox;

    1
    Davis•330
    @davislocsPosted almost 2 years ago

    Hey whats up!

    I have a few suggestions for you to improve your code:

    • Dont use<div>for everything. There are many other tags whitch you can use for better accessibility. Before even trying to make this responsive, first learn about proper semantic meaning about html tags.

    Whatch this video about correct use HTML semantics.

    • for responsivnes use :
    @media only screen and(max-width: 800px)
    • I also suggest to start using rem units instead of px:

    here are a video about units.

    I hope this helps. Happy coding! :)

    0
  • Davis•330
    @davislocsPosted almost 2 years ago

    Hey whats up!

    I have a few suggestions for you to improve your code:

    • Dont use<div>for everything. There are many other tags whitch you can use for better accessibility.

    For example insted of this:

    <div class="card"></div>

    Use this:

    <main class="card"></main>

    Whatch this video about correct use HTML semantics.

    https://www.youtube.com/watch?v=kGW8Al_cga4

    • I also suggest to start using rem units instead of px:

    here are a video about units.

    https://www.youtube.com/watch?v=N5wpD9Ov_To

    • Always use alt attribute on <img> to specify an alternate text for an image, if the image cannot be displayed.

    Happy coding! :)

    Marked as helpful
    1
  • Davis•330
    @davislocsPosted almost 2 years ago

    Hi Cindy! your project looks great, but i have one suggestion for you:

    • Use <picture> tag insted of <section> for wraping all of your pictures.

    Insted of this:

    <section>
      <img src="images/image-product-desktop.jpg" class="product-image" alt="Perfume image Gabrielle channel">
</section>

    Do this:

    <picture>
        <source media="(max-width:800px)" srcset="images/image-product-mobile.jpg">
        <img src="images/image-product-desktop.jpg" class="product-image" alt="Perfume image Gabrielle channel">
</picture>

    This will help you with changeing pictures from desktop to mobile version. Instead of having one image that is scaled up or down based on the viewport width, you can use multiple images.

    If you found this helpful please mark it as such :)

    Marked as helpful
    1