Skip to content
OverviewSolutions
26
Comments
11

Sounak Mukherjee

@kanuosNew York560 points

I love programming, solving sudoku and learning about programming paradigms.

Latest solutions

View more solutions

Latest comments

  • Sounak Mukherjee•560
    @kanuos

    Congrats on your first submission. It looks great :)

  • Sounak Mukherjee•560
    @kanuos

    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 :)

  • Sounak Mukherjee•560
    @kanuos

    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.

  • Sounak Mukherjee•560
    @kanuos

    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
  • Sounak Mukherjee•560
    @kanuos

    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?

  • Sounak Mukherjee•560
    @kanuos

    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
View more comments