Skip to content
OverviewSolutions
47
Comments
24
ROCKY BARUA
@Drougnov

All comments

  • ROCKY BARUA•1,090
    @DrougnovPosted about 2 years ago

    Hello @cnsacramento, great job. The code looks neat and clean. Here are some things you can improve:

    • Change the h2 to h1 as the page should contain a level-one heading.
    • The box shadow is not working on the .card, cause you've only provided the color. You also need to provide the h-offset and v-offset value. You can learn more about box shadow from here
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted about 2 years ago

    Hello @ilvdrskn, great job on completing this challenge.

    As the .modal-menu is an absolute element and its parent .head doesn't have a defined height, it is only taking the head's initial height. To fix this, simply remove the height: 100% from the .modal-menu, and it will get the desired full height.

    Also, try to fix the accessibility warnings. If you need further help on this, feel free to ask or check out my solution. Have a good day :)

    Marked as helpful
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted over 2 years ago

    Hi @SpacemanOG, the design is looking great. Good job.

    Use a pseudo-element(::after,::before) to create an overlay on that image container. Like this:

     .heroImage::after {
  position: absolute;
  content: "";
  width: 100%;
  height: 100%;
  background: hsla(277, 64%, 40%, 0.6);
  top: 0;
  left: 0;
}

    And don't forget to add position: relative to the heroImage.

    You can remove the 'source media' for the 300px width as the img will be shown by default on lower that 1200px width screen.

    Hope this helps. Have a good day :)

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 2 years ago

    Hello @cenkderman, great job.

    • The design is looking perfect on the desktop viewport. But the text-content is exceeding the container in mobile view. Avoid using height if not absolutely necessary.

    • The google font link is wrong. In the HTML change your font link to this:

    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Fraunces:opsz,wght@9..144,700&family=Montserrat:wght@500;700&display=swap" rel="stylesheet">```.

Or, just simply add this line at the top of your CSS :

```@import url('https://fonts.googleapis.com/css2?family=Fraunces:opsz,wght@9..144,700&family=Montserrat:wght@500;700&display=swap');```
    • Use a default font in the body tag. Ex: Montserrat and use the other one on the needed element. In this way, you can save some lines.
    • Although you have beautifully added the images with display none and media queries, the semantic way would be using the picture tag. Like this:

    <picture> <source media="(min-width: 650px)" srcset="./images/image-product-desktop.jpg"> <img src="./images/image-product-mobile.jpg" alt="Gabrielle CHANEL's perfume bottle"> </picture>

    Don't forget the alt text 😉.

    You can check my solution for further information. If you have further questions, feel free to ask. Have a good day.

    (edit) Having trouble formatting the comment. I hope you can still understand what I'm trying to say

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 2 years ago

    Hi @caio-rosa, good job on the challenge.

    To achieve the purple filter on the image, you can use pseudo-code(after or before) on the container. At first use position relative to the container.:

    .container {
  position: relative;
  z-index: 0;
  overflow: hidden;
}

    And add after to the container like this:

    .container::after {
  position: absolute;
  content: "";
  width: 100%;
  height: 100%;
  background: hsla(277, 64%, 40%, 0.6);
  top: 0;
  left: 0;
}

    this should do the trick.

    You might wanna work on the accessibility and responsiveness(mobile devices). For further help feel free to comment or view my solution

    Anyway, keep up the good work and have a good day : )

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 2 years ago

    Hey @caiomiyaji, good work. The card looks awesome.

    You can fix the accessibility issue by changing the h2 to h1. It is the best practice to have a level 1 header for a well-designed page.

    Here is a blog about the details if you wanna know more.

    Marked as helpful
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted almost 3 years ago

    Hello @pharaohmak, good job on the design. Still, you can make a lot of improvements to your codes. Here are some suggestions for you:

    • Your HTML isn't semantic. To fix that you can use some tags like header main section footer instead of div.
    • The card should be in the center of the page. Add justify-items: center on the container to move it to the center.
    • The background image is not aligned well. To fix it use 
       hsl(225, 100%, 94%);
background-size: contain;
background-position-y: -15%; ```

    on the body.

    • You have only downloaded font-weight 200. That's why the heading isn't bold. Re-download the font or use this @import url("https://fonts.googleapis.com/css2?family=Red+Hat+Display:wght@500;700;900&display=swap"); in your CSS.

    Have a good day my friend :)

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @RikkaaDev, great design. As you asked, yes, it is actually a bad practice. We should only put the logo and nav (if needed) inside the header. The actual content should be wrapped around the main tag. If you want to explore more here is a blog about when to use header

    The design looks great but the image looks a little bit small on mobile devices. Use width:100% with media queries to give it full width.

    Again, great job. Have a good day my friend :)

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @devenyam, great job. The design looks pretty good.

    However, you can improve on the responsiveness part(especially on laptop devices).

    • Use max-width: 320px(any size) instead of width. It will look more flexible. You'll not even need a media query then.

    • There are some accessibility issues on the Html part. Use main, footer, section etc. instead of div. And use at least one h1 tag on the page. You can change the h2 to h1.

    Overall, great design. Keep it up. Have a good day my friend :)

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @NicolasGula, you did a marvelous job on the design. It's perfect.

    However, you need to improve your Html code's accessiblity. Change/Add these things:

    • Use main, footer, section, article etc. instead of div.
    • Use responsive unit like rem, em instead of px
    • You need atleast one heading in your page. You can change the <p class="title"> to h1 class="title"

    Overall, the design looks great. Keep it up. Have a good day, my friend :)

    Marked as helpful
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @cyberspatial, great job. You can add the patterns by using pseudo-classes(:: before,:: after) to the body. Try this:

      body{
  position:relative
  }

  body::before {
  background: url(./images/bg-pattern-top.svg) no-repeat bottom right;
  top: 0;
  left: 0;
}

body::after {
  background: url(./images/bg-pattern-bottom.svg) no-repeat top left;
  top: 100%;
  left: 100%;
}

    It's probably the most efficient way. Also here are something you might wanna consider to change/add:

    • The card looks much wider than the actual design. So add max-width to the card to fix that.
    • You need to start with level one heading(h1). That means you cannot start with lower(h2,h3, etc). Don't use the section or article tag if there is no child heading. Use div instead.

    Overall your design looks great. Have a great day :)

    Marked as helpful
    2
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @mroungou, Great job on the implementation.

    Although, here are some things you can improve.

    • Your hover effect works fine both on the change link and the button but you need to change the color too. So add color: hsl(245, 75%, 62%); and background-color: hsl(245, 75%, 62%); respectively in the hover rules.
    • You need to add a background pattern as the design required. Use background: url(./images/pattern-background-desktop.svg) no-repeat, hsl(225, 100%, 94%);
    • Use max-width instead of width and rem instead of px for responsiveness. Then you wouldn't even need media queries for the card.
    • To fix accessibility issues, use header main footer etc. instead of div. And make sure to use at least one heading on a page.
    • Change the h3 to h1 as you need at least one level one heading on a page.

    If you need more information or help, feel free to ask or check out my solution. Have a good day my friend :)

    Marked as helpful
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted about 3 years ago

    Hello @dombrga, Great job. I like how you added different error messages for empty and invalid emails. And good job on the responsive part too.

    However here are something you might wanna consider:

    • The Facebook icon doesn't match the size of the other icons. Add some padding to it.

    • To fix accessibility issues, add aria-label='something like this' to those a tags.

    • To fix Html issues, use div instead of section. Cause you need headings while using section or article tags.

    Overall, it's looking great. And it's my favorite solution to this challenge. Have a good day :)

    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 3 years ago

    Hi @catherineisonline, great job with the layout and responsiveness.

    However, here's something you may consider:

    -The background pattern isn't flexible enough. Use background-size: cover to fix it. -The Facebook icon doesn't look good. Add some padding to both sides.

    Overall, great solution. Happy coding :)

    1
  • ROCKY BARUA•1,090
    @DrougnovPosted over 3 years ago

    Hello, @Pat-Gekoski great work.

    Just use max-width:111rem instead of width:111rem. It will make the card shrink as the screen width shrinks.

    Marked as helpful
    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 3 years ago

    Hello @ayushv45, Great job. It's looking good.

    However, here are something you can do to make it even better:

    • The pattern doesn't look good in 375px or lower width devices. Use a different background image for mobile devices using media queries. It is provided in the images folder.

    • Use the blue-ish color on the headings as the design requires.

    • Try to avoid div whenever you can. Use main directly on the container class and the footer on the attribution class. It's better for accessibility.

    Good luck with the next solution and Happy coding :)

    Marked as helpful
    1
  • ROCKY BARUA•1,090
    @DrougnovPosted over 3 years ago

    @dombrga yes sir, it is close enough. You did a marvelous job.

    Everything is perfect except the background pattern. It's not flexible enough for bigger screen. You can fix it by adding background-size: contain.

    Change the h2 to h1 to make your code accessible.

    Plus you need to add a different pattern for mobile devices. But the design is pretty good. Great job. Happy coding :)

    0
  • ROCKY BARUA•1,090
    @DrougnovPosted over 3 years ago

    Great job @SachinShelke7. It looks wonderful.

    Just lower the opacity of the hover color a little bit.

    1