Skip to content
OverviewSolutions
26
Comments
11
Sounak Mukherjee
@kanuos

All comments

  • Brian Nelson•150
    @briannelson95Submitted over 1 year ago

    This was a pretty easy challenge for me. I decided to keep it simple and utilize just html and css files.

    QR Code Component

    2
    Sounak Mukherjee•560
    @kanuosPosted over 1 year ago

    Congrats on your first submission. It looks great :)

    0
  • Sounak Mukherjee•560
    @kanuosPosted over 1 year ago

    Hi @Kuzitaa, I previewed the site and found some issues - mostly JS issues.

    1. The form can be submitted with all/some unfilled form fields.
    2. The validation is not working e.g. - the card holder name should not have digits etc
    3. Since you're using client side validation - the form submission's default behavior should always be prevented - unconditionally
    4. A form is submitted only if the button has a type="submit" and not type="button". In your HTML form, update the button type.
    5. You used button.addEventListener('click', completeFormView) on line 18 as the form doesn't submit on click meaning the form validation is never run. It's a hack and not a good one too. You cannot bypass validation
    6. There is not one heading element - try to include a h1 element per page - even if you have to hide it using CSS
    7. Try to use any CSS naming convention e.g BEM and structure your tags and classes accordingly.
    8. Using span to denote the thank you logo is not preferred. It should be an img element.
    9. You are using absolute units in your style - e.g width: 280px;. Using relative units helps in avoiding unnecessary media queries.
    10. Also, if you can - write modular js files and unit test them. It will help you a lot

    Apart from these, I think your solution is pretty decent. Fix the issues and re-upload if you can.

    Let me know what you think of the feedback - happy coding :)

    1
  • caduovieira•30
    @caduovieiraSubmitted over 1 year ago

    Anchor tag error

    When I was finishing my project I found the following error:

    • Link 'rel' attribute should include 'noopener'.

    The code was written as follows:

    <footer>

  Challenge by <a  
    href="https://www.frontendmentor.io?ref=challenge"  
    target="_blank">Frontend Mentor</a>

  Coded by <a  
    href="https://github.com/caduovieira"  
    target="_blank">Cadu Vieira</a>

</footer>

    If someone could help me explaining why this happened, I'd be thankful.

    Interactive Rating Component - Sass & jQuery

    1
    Sounak Mukherjee•560
    @kanuosPosted over 1 year ago

    Hi Cadu, it is not an error - rather an attribution. When someone sees your solution, FrontEnd Mentor wants the visitor to know that the design was created by them.

    If you want you can hide them using something similar to tailwind's sr-only class.

    1
  • Sounak Mukherjee•560
    @kanuosPosted over 1 year ago

    Hi Lesya, congrats on your first submission. The UI structure looks nice, however I've few questions/suggestions -

    1. Why is the card heading replaced by random lorem ipsum texts?
    2. Same goes with the card description
    3. Imagine a scenario - you visited a website where you are to scan a QR code (preferably by a phone camera). Would you like to hover the card before you can scan? There's no need for this effect. It is counter productive as it beats the purpose of a Quick Response card. The card rotation using perspective property is cool but kinda goes against the UX of this app.

    As far as the semantic HTML and BEM naming conventions are concerned - you aced it! At a quick glance, I thought it was perfect. Keep up the good work :)

    Marked as helpful
    2
  • Sounak Mukherjee•560
    @kanuosPosted over 1 year ago

    Hey Khaled, Your design is not mobile responsive. It looks good on desktop but the mobile UI is completely missing. Try to fix that and generate a new screenshot.

    Also, I found this in your css file -

    body {
  background-color: hsl(0, 0%, 81%);
  font-family: "Bai Jamjuree", sans-serif;
  font-family: "Barlow Semi Condensed", sans-serif;
  font-family: "Big Shoulders Display", cursive;
  font-family: "Fraunces", serif;
  font-family: "Karla", sans-serif;
  font-family: "Kumbh Sans", sans-serif;
  font-family: "Lexend Deca", sans-serif;
  font-family: "Montserrat", sans-serif;
  font-family: "Open Sans", sans-serif;
  font-family: "Outfit", sans-serif;
  font-family: "Poppins", sans-serif;
  font-family: "Raleway", sans-serif;
  font-size: 10px;
}

    Could you explain why you did that?

    0
  • Kamil Wiater•230
    @WiaterKaSubmitted almost 2 years ago

    Had problems with a proper positioning of the image (please see the bottom line).

    Any feedback concerning the above (and other aspects) hugely appreciated :))

    Perfumes Preview Card

    3
    Sounak Mukherjee•560
    @kanuosPosted almost 2 years ago

    Hi Kamil, congrats on your submission. Your code looks pretty decent to be honest but like they say - there's always room for improvement.

    Here's a few tips on how to improve your code

    1. Your code is missing the landmark wrapper. Easy fix - convert the div.container to main.container
    2. Looking at your class names, try to follow a standard - eg. BEM. Look up BEM naming convention. That'll help you isolate the UI into minute re-usable components - hence leading to DRY-er code.
    3. Always use an alt attribute to your img element.
    4. Try to make the img styling more generic. The starter kit provided by FEM is kind enough to resize and optimize the images. What if the images were out of shape/aspect ratio? Use object-fit object-position etc props on img elements. Setting the display to block helps prevent a lot of unnecessary bugs.
    5. If you look at the design, you'll see the font weight of the heading element is not optimal in your code. Fix it when you can. While at it, fix the font size of the card description too.
    6. You probably missed out the hover effect on the button. As a pointer, I'd recommend using a <a> tag instead of the button element. Also, decrease the border-radius of the button to make it look identical to the design's button.
    7. If you look at the desktop design, the image on the left and the card info on the right are of the same width. In your solution, the image looks a bit thinner compared to the card info. Also, the padding on the text content is a bit more in the design.
    8. Try using relative units like rem, %, em etc instead of absolute units like px.

    Hope this helps. Hope to see a lot more from you in the future. Happy coding :)

    Marked as helpful
    1
  • Sounak Mukherjee•560
    @kanuosPosted almost 2 years ago

    Hi @petronela88, congratulations on your first submission. Your solution looks decent however there's room for improvement. Here's some of the things you should fix and regenerate a new screenshot -

    • Convert the div.container to main.container - that will resolve your landmark issues
    • Add a lang="en" attribute to the HTML
    • Convert the h3 tag to h1
    • Hide the footer and it's contents as they are not there in the design. Hide them using CSS, so that they are available for the screen-readers to read. Look up screen reader classes to do so. Here's a link if you want to read more about it.
    • The entire heading and text should have text-align: center as per the design.
    • Color of the h3 tag in your code looks a bit darker than the design
    • Reduce the padding/margin below the h3

    Hope this helps.

    Hope to see more of your submissions soon. Happy coding :)

    Marked as helpful
    1
  • Sounak Mukherjee•560
    @kanuosPosted about 2 years ago

    Hi @vishwam, congrats on completing this project. However, your app isn't mobile responsive. Also you missed out the rounded corners. Try to fix them and generate a new screenshot!

    Marked as helpful
    0
  • Igor•70
    @IgorM867Submitted about 2 years ago

    What could I improve on this simple project?

    HTML and CSS

    3
    Sounak Mukherjee•560
    @kanuosPosted about 2 years ago

    Hi @Igor, the end product looks great! Congratulations on your first submission.

    However, a few things you might want to consider :

    1. Text below the Heading should be of the lighter color than it is now
    2. A bit less padding on the card would make it look closer the to design
    3. Instead of using generic div element to wrap the card, use a semantic tag like section or main.
    4. It's always better to specify the object-fit property of an img as well as making it's display to something other than the default inline value.
    5. Leave the default attributions in the HTML for screen readers. Hide them using CSS.

    Overall, it's a pretty decent effort! Happy coding!

    Marked as helpful
    0
  • Sounak Mukherjee•560
    @kanuosPosted about 2 years ago

    Hi @Tachy-22, you haven't added the font family to your design as per the style-guide.md.

    Also, why are you using background image to display the qr code element. <img/> tag should be used as the qr code is the main element of the card. Background images are used only for decorative purposes.

    Try using relative units e.g. rem, em, % instead of absolute units like px.

    Hope these feedback help you :)

    Marked as helpful
    1
  • Adriano•38,230
    @AdrianoEscaraboteSubmitted about 2 years ago

    👨‍💻 Hello guys.

    This is my solution to this challenge. It was a lot of fun and challenging to do this project, at times I had complications with the positioning of the hero image, but I think I did a good job. I would like to thank my friend @correlucas for giving me the opportunity to do a premium project.

    Tamo junto man 🇧🇷

    I added some details:

    • 🎨Dark mode and light mode
    • 👨‍🎨Images hover effect

    Feel free to leave feedback on how I can improve my code. 😊

    Thanks!

    Skilled elearning landing page with DarkMode (HTML + CSS + JS) 👨‍💻

    4
    Sounak Mukherjee•560
    @kanuosPosted about 2 years ago

    @correlucas Hi Adriano, your design looks impeccable. However, as you probably have observed, the design has a prominent linear-gradient background which fades from bottom up, whereas in your solution that gradient isn't that prominent. Was that something you did deliberately as your own variation?

    1