Skip to content
OverviewSolutions
20
Comments
19
Basant Soni
@SoniBasant

All comments

  • Basant Soni•450
    @SoniBasantPosted over 1 year ago

    You will see red colour border with exclamation circle when input is wrong.

    You will see green colour border with check mark circle when input is wrong.

    Also, password strength should be good. : )

    0
  • Basant Soni•450
    @SoniBasantPosted over 1 year ago

    This is great.

    How do you get exact sizes- padding, margin, font-size, width and most important height of the page?? Many of my challenges don't look good due to wrong sizes. Please help me out. : (

    1
  • Basant Soni•450
    @SoniBasantPosted almost 2 years ago

    Good work. 👏👏

    I am making same. So I don't have suggestions, I have questions if you please answer. 🙏

    • How do you get white footer logo and also head logo for fevicon. These are not in my download file. 🙁

    • I was given svg images for social media icons so having difficulty in changing colour on hover. And you used i icons. 😇 That's clever. But will be guide me how to change colours on hover while using svg. Till now no success.

    2
  • Basant Soni•450
    @SoniBasantPosted almost 2 years ago

    When I first saw this in comparison box, I thought why you did this!!

    Then I saw the that button and realized what you did there. 😀

    Colour selection for light theme is good.

    Good job. 👏👏

    PS: I need to follow some advice from advice generator. Also, need to make some projects in Frontend Mentor. 🏃‍♂️🏃‍♂️

    1
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Hi, Mate. 🙋‍♂️

    Your project is looking good.

    As for the question of flex OR grid, I recommend -

    • Flex-box when you have components in one line either in one row or one column.

    • Grid for more than one line. You will have rows and columns which are easy to handle in grid.

    Hope this is helpful. 🙂

    1
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Hi, 👋 Your project is good.

    As for your question of center a div-

    display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;

    Use above code in body.

    Remove min-height: 100vh; if using elsewhere.

    • Use text-align: center; in p and h5

    • Change h5 to h1 as one heading is necessary.

    • Wrap your div of class="card" in <main></main> tag to remove landmark error.

    • Wrap your div of class="attribution" in <footer></footer> tag to remove landmark error.

    Hope I am helpful. 🙋‍♂️

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Hi, 🙋‍♂️You made a good project.

    I wanna help you to reduce accessibility report and your question of course.

    As for your question regarding text colour 🎨-

    • Use Dark-blue: hsl(218, 44%, 22%); for "Improve your frontend..."
    • Use Grayish-blue: hsl(220, 15%, 55%); for "Scan the QR code..."

    Now about your report 📝 you got; In semantic coding, we need to take care of-

    • Landmark - Change <div class="Qr"> to <main>. This is for landmark problem. There should be one landmark. Don't forget to change closing tag.

    • Level-one Heading - There should be at least one heading. So change <p class="text"> to h1. No need to give class and handle font-size in CSS.

    • Use padding in place of <br>. This will improve your dimension problems.

    • Avoid px in CSS as much as possible. This creates problem in responsiveness. Instead use rem. 1 rem = 16px.

    Hope I am helpful. Feel free to ask if you have more questions. 👨‍💻

    1
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Your project is so good that I couldn't found anything to give feedback in desktop view. So I switched to mobile view.

    You don't use media query at 375px.

    As given in the style-guide, Mobile Version should be at 375px.

    In that image of mobile-design.png, p have 4 lines, your project have 5. It means you need to work on padding and font-size to make it as per design.

    Other than that, very good project. Great work : )

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Your hard work shows in this project. Very good job.

    As for the suggestion -

    • Use h1 for <h2>SEDANS</h2>. One heading is necessary in semantic coding. That's why the warning Page should contain a level-one heading. You can handle font-size in CSS.
    • Use flex in body so your solution will be at center.
    body {
    min-height: 100vh;
    display: flex;
    align-items: center;
    justify-content: center;
}

    Other than that no suggestion. Good work. : )

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Hello mate, You made a decent card solution. It also have some scope to improve.

    • Change <section class="main-body"> to just <main>.
    • As given in style-guide.md -

    Layout

    The designs were created to the following widths:

    Mobile: 375px

    Desktop: 1440px

    So use media query at 375px. You used at 600px.

    • Change h3 to h1 then change font-size of h1 as you need from CSS. As one heading (h1) is necessary.

    • Use Dark-blue: hsl(218, 44%, 22%); for h1

    and Grayish-blue: hsl(220, 15%, 55%); for p

    • Change border-radius for .outer-div and .outer-div img to make them more similar as you can see in design comparison. You can do this in .outer-div img. Remove padding-top.
    .outer-div img{
    width: 88%;
    border-radius: 10px;
}

    Hope I am helpful. Good Luck. : )

    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Hey Sheetal,

    I made some changes and send a pull request to you in your GitHub. Please see this and and merge it. This will improve your code.

    1. It is better if you use a separate file for css as style.css and add link in head section of html file. It is easy to write, maintain and debug. I add a commented link in your html file for this. Just copy all that is inside of style tag and paste it in style.css file
    2. Don't use dot (.) with body.
    3. Use background-color in body.
    4. Use flex properties in body so you don't need to use width: 70vw; and height: 80vh; in .main
    5. Use rem in place of vw in .main-content for width as width: 15rem;
    6. Use main in place of <div class="main">.
    7. Change p of "Improve your front-end..." to h1. As one heading is necessary.
    8. Use 375px in place of 400px in media query.
    9. Don't use width: 70vw;, height: 80vh;, margin: auto auto; and margin-top: 60px; in main. As card is in the center and we are using flex for this.

    As this is your first solution here, you learned a lot. Keep going. Keep learning.

    If you need more help, please feel free to ask. : )

    Good Luck.

    You can check my solution of this project in GitHub if you wanted.

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago
    • Add flex in .product-price, so that your .product-price p will be at center as compared to .product-price h1.
    display: flex;
flex-direction: row;
align-items: center;
    • Add line-height in .preview-product p. Also adjust font-size in this so that second line end with "by", third line end with "Creater". And there is no fifth line. So work on line-height, font-size and paddings in h1, h2 and p.

    Size of full card is exactly same. Kudos to you for this.

    Good work : )

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago
    • You forget to use . with text in p. Text didn't become class. That's why the color of "Scan the QR code.." line is not as you intended.

    Such mistakes happens. Don't worry. : )

    I made a pull request in GitHub regarding this.

    • Also, change padding of p, so that second line end with "skills to" NOT with "the".

    • Change h2 to h1 for mandatory heading.

    • Wrap all content in body with main tag.

    Other than that you did great job. 👏

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago
    • Use below code in body
       display: flex;
    align-items: center;
    justify-content: center;

    Your card will be centered.

    • At least one h1 heading is necessary, so change h2 into h1 and give font-size as needed.

    • You don't add language. Add it in html tag-

    <html lang="en">
    • You don't use <!DOCTYPE html> necessary for browser and <meta> tag contains information about html document. As these are already present in the html file given with this project, means you did not use that file.

    If you feel you don't need it, it's totally fine. You can type these lines.

    OR

    If you are using VS Code you can just type ! in html file and select first option from pop-up. And Bang! your boilerplate is ready

    • You don't add fevicon. Image is given with the project. Add the link tag in head.

    • I made some changes in your html file and css file in GitHub. Please have a look. And accept pull request if you feel good.

    Good Luck Fabio F. : )

    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago
    • Don't use % with width. % is used when you write it in CSS. In HTML, % is not required. I suggest to remove it completely and handle style in CSS.
    • Change .article_title to h1 instead of p, so that you can have at least one mandatory heading. Change font-size as you need.
    • In first para, Shift the overall..., First sentence should end with by and second with to.
    • You choose wrong color for date-month-year. You use color: hsl(214, 17%, 51%); I think you should use color: hsl(212, 23%, 69%);
    • No or very light box-shadow with image. Check with the image you downloaded or in the comparison given above. It is only with content.

    Placing of the image of person and name is good. Other paddings are also good. Overall card is looking great.

    Good Job. : )

    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Make it larger, center it using flex, convert h2 into h1 then bring it size down.

    All other are good. Good Luck. : )

    Marked as helpful
    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    This is very good. : )

    0
  • Basant Soni•450
    @SoniBasantPosted about 2 years ago

    Use "flex-direction: column" in body. So your name and QR code will be at center.

    Use font style and background color as given in the style-guide.

    Use border radius in image of QR code.

    Other than that your project is good. With my suggestions, your project will look really good.

    Good Luck. : )

    0