Skip to content
OverviewSolutions
21
Comments
16
Rachit Patel
@rachit0706

All comments

  • Rachit Patel•540
    @rachit0706

    Also, you can use <picture> element for different screen sizes instead of media queries. Like below-

    <picture>
  <source media="(min-width: 600px)" srcset="desktop.jpg">
  <img src="mobile.jpg">
</picture>
  • Rachit Patel•540
    @rachit0706

    Congratulations for completing the challenge! . Everything looks great but the only issue I found was that you forgot to use flex-direction: column; inside the body tag. This will properly center the component in the page and will match the design.

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Congratulations for attempting the challenge! But there are a few suggestions with which you can improve your code

    • For creating gaps in your text wrap them inside a div and use flexbox and I assume that you know about flex as you have already used in the body tag.
    • I think you have forgotten to push the images in the github repository because they are not showing in the live site.
    • Also, the main heading of 'Equilibrium #3429' should be wrapped under h1 tag instead of h2 for better semantics and seo. You can adjust the font size of this h1 according to the design.

    If you find this feedback valuable, then please mark it as helpful

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Try using Netlify for deploying the project. It's very easy and requires few clicks. Below are the links for reference -

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    No offence buddy but instead of doing this cheap trick of uploading a blank solution to just get the points, you should actually try to build the project by referring to documentations available on the internet. You may not be able to build the best project right from the start. But it will help you in the long run . Doing tricks like these actually degrades the quality of community and also disrepects the hardwork put by so many members while doing the challenges. I hope you get my point.

  • Rachit Patel•540
    @rachit0706

    Congratulations for completing the challenge!

    I do have a suggestion for you to improve your code. Your body element is not taking the full height of the viewport and the component also is not at the center of the page as depicted in the design. For that you can use the following CSS in your code.

    body {
    min-height: 100vh;
    display: grid;
    place-items: center;
}

    This will ensure the body takes the proper height and component will be at center.

    Also instead of using <h3> tag for heading you should use <h1> as it is suggested in the accessibility report . You can then give it a proper font-size.

    If you find my feedback valuable then please mark this as helpful. Cheers!

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Congratulations for completing the challenge

    I have a suggestion for you regarding the image. Insteade of using display : none in the media queries you can use the <picture> element. It provides the feature of providing different images for different screen sizes.

    Here is the link for it's reference Picture element reference W3Schools MDN reference

    If you find this feedback valuable please mark this as helpful Cheers!

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Congratulations for completing your first challenge!! There are few suggestions for improving your code:

    1. Wrap the whole content inside a main tag for better semantics.
    2. You can use flexbox with flex direction as column on your main tag to center the qr component in the page. Here are some helpful resources: https://css-tricks.com/snippets/css/a-guide-to-flexbox/, https://www.w3schools.com/css/css3_flexbox.asp https://flexbox.tech/
    3. Using flexbox in the card will also fix your layout issue and then you will not need to set position attributes in the card.
    4. You can set font family in the body selector itself instead of repeatedly setting in sentence 1 and 2.

    If you have any more doubts you can reply to this feedback.

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Congratulations @alexdeanb for completing this challenge! I do have a few suggestions for you:

    1. First of all wrapping your content inside a main tag for better semantics. Don't directly give the body element so many styles. Give the styles like flex to your main element.
    2. You can use flexbox with column direction for your card component. It will give a better layout. Use justify conent as center like you did for body.
    3. Doing the above might fix your accessibility issues as well.
    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Did you submit this just for getting points?

  • Rachit Patel•540
    @rachit0706

    Congratulations for completing the challenge! But I have a few suggestions for you:

    1. You should use a seperate css file for writing all your styles and then link it to the html.
    2. You have put wrong path in the img tag src attribute. Because of this the image is not rendering. 3 .Your body is not taking the whole width and height of the page. Set padding and margin to 0 and give min-height as 100vh to body tag.
    3. You shouldn't make the body as flex box container. Instead instead use main element inside body and then a div to wrap the content. Then you can set the display as flex for that container div.
  • Rachit Patel•540
    @rachit0706

    Hey Naitik great attempt of the challenge! But I have few suggestions for you for your code:

    1. You should use a semantic element like main and put every element inside.
    2. You can add a common class like "card" or something else to every card and use flexbox for better layout. Here are some useful links: https://www.w3schools.com/css/css3_flexbox.asp, https://flexbox.tech/
    3. Instead giving different border radius to each card you should give one border radius to the container div and set its overflow property to hidden.
    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Great attempt but only one thing you missed was creating box shadow while hovering on the button and it seems you have some accessiblity issues which you can fix.

  • Rachit Patel•540
    @rachit0706

    Hey, It was a great attempt but I found a few issues in your solution:

    1. Your component is showing the thanyou phase even if a rating is not selected. You should alert the user if they have not selected a rating.
    2. I would suggest you to use flexbox for the "buttons" div so that the rating buttons can be properly placed in a row.
    3. Your design is not mobile friendly. Use media queries for that.
    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Great work,

    But a suggestion for you will be to use relative lengths for height and width like rem, % etc rather than absolute lengths units like px because design is not responsive in mobile view

    Marked as helpful
  • Rachit Patel•540
    @rachit0706

    Thank you very much Lucas for sharing your valuable feedback. I read your suggestions and have made some changes. Please take a moment to again look on the project (preview site and code) and give any any more suggestions you have regarding this.