Skip to content
OverviewSolutions
46
Comments
46
Akhlak Hossain Jim
@Akhlak-Hossain-Jim

All comments

  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted almost 2 years ago

    Hey, You have done a great job completing the challenge. Here is some of my suggestions to make it more better.

    • remove the margin from all side in the mobile view (margin: 0;)
    • In the mobile view try adding width: 100%;
    • You can start your from 576px (standard mobile view of bootstrap)
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted over 2 years ago

    Hi there, great work so far,

    But here are some things you can do to improve your code.

    1. add line-height: 100%; at your .rare-btn that way it will be a proper circle.
    2. add can add an if else statement to check if someone has selected a rating or not, if so you change the card or do nothing that would be great

    Keep up the good work. Happy coding:)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted almost 3 years ago

    Great work so far.

    Here are some of my suggestions,

    1. consider using column gap as well for responsiveness
    2. instead of hard-coded values like px, try using relative values like em, rem, % etc making the site responsive

    And remember practice will make it better so practice more. Happy coding :)

    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted almost 3 years ago

    Great work so far. Everything looks good on the website.

    Here is some of my suggestion,

    1. I found % is best for this kind of scenario where calculation involved
    2. and for the resolution, you should consider using relative values like em, rem, and more or you can use CSS functions like min(), max(), minmax() check this and this resources for reference.
    3. check for typos in code like sectiom

    Keep coding everything just need a bit practice. Happy coding :).

    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted almost 3 years ago

    Great work so far, but

    1. consider using main tag for the card container that would be a great practice
    2. add some space in the mobile view
    3. try using h1 at first and then h2, h3 and all

    Other than that everything looks good. Happy coding:).

    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted almost 3 years ago

    Nice work. For that you can do:

    1. set a fixed bottom padding,
    2. take a div with display: flex; for p and the button and
    3. make flex-direction: column; justify-content: space-between;

    or

    1. in section add display: flex; flex-direction: column;,
    2. select img and add width:64px;height:auto;
    3. in button add margin-top:auto;

    and it should always stay in the same position.

    Happy coding :)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Hey, Great work so far,

    add title attribute to the i tag to solve the accessibility issues.

    Happy coding :)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work so far. But here is some of my suggestions to fix your issues,

    • try adding alt text in the image and try adding in every image.
    • remove action attribute for now as you are not doing anything with that. So remove that your issues should go away.

    Happy coding:)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Hey Matt, I just reviewed your code and preview, great work so far, and here are some of my suggestions:

    • try to add some padding in the .main-space class.
    • instead of div in <div class="main-space"> try using main tag here.
    • instead of <div class="attribution">Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>.Coded by <a href="#">Your Name Here</a>.</div> use <footer class="attribution">Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>.Coded by <a href="#">Your Name Here</a>.</footer>.
    • try using CSS variables.

    Happy coding :)

    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    hey, renzzzz. great work so far.

    I just saw the preview and the code, here are some of my suggestions.

    • try first heading as h1
    • try learning and using CSS variables.
    • try learning Sass or some frameworks/libraries.

    Happy coding :)

    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work adam. I just saw through your code and the live preview. It looks great. Here is my feedback:

    • try not to use div directly in the body, like <div class="social-container"> use header main whatever suits best in that case.
    • try using CSS variables for color and stuffs.

    Happy coding:)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work Andy. I just saw your code and live preview. It looks so good to me just try doing the following:

    • add some top padding to the body it looks stacked in the mobile view.
    • try leaning new things like sass/less and/or css frameworks.

    Happy coding :)

    Marked as helpful
    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work Victor,

    I just saw the preview and your code. It looks so good to me. So I guess it's time to learn new things, try learning these:

    • SASS/Less
    • css frameworks like bootstrap, tailwind css etc.

    Happy coding.

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work Ranzo. Her is my recomandations to solve some of your issues.

    • try wraping <div class="attribution">Challenge by <a href="https://www.frontendmentor.io?ref=challenge" target="_blank">Frontend Mentor</a>. Coded by <a href="#">Renzo Alvarez</a>.</div> this in a <footer> tag.
    • don't use <section><main>...</main></section> instead use <main><section>...</section></main>.

    your other html css code looks good to me so try learning sass and frameworks to move forward.

    Happy coding :)

    Marked as helpful
    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work Ryan. Try these recomandations:

    • your code
    <footer>
      <div class="attribution">
        Challenge by
        <a href="https://www.frontendmentor.io?ref=challenge" target="_blank"
          >Frontend Mentor</a
        >. Coded by <a href="https://github.com/RyanFloresTT">Ryan Flores</a>.
      </div>
    </main>
</footer>

    Should be

    </main>
<footer>
      <div class="attribution">
        Challenge by
        <a href="https://www.frontendmentor.io?ref=challenge" target="_blank"
          >Frontend Mentor</a
        >. Coded by <a href="https://github.com/RyanFloresTT">Ryan Flores</a>.
      </div>
</footer>
    • try using css variables

    Happy coding:)

    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work so far.

    • instead of <div class="card"> use <main class="card">
    • it's not responsive now try doing that, and code not accessible.

    try those fixes and mark the comment helpful if it is.

    Thanks, Happy coding :)

    Marked as helpful
    1
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    Great work so far.

    But

    • add a dummy text in the h1 it can be ADVICE # but put it there.
    • instead of using <p id="advice-text"> use <q id="advice-text"> that's a accessibility best practice.

    Happy coding :)

    0
  • Akhlak Hossain Jim•1,325
    @Akhlak-Hossain-JimPosted about 3 years ago

    You have done great work so far.

    Try implementing these:

    • add alt attribute in images and don't keep it empty.
    • go h1 to h6 serially, meaning use first heading tag as h1 then if you need to go down, do not go to h3 or h4 go to h2 first then following and repeat h1 or any one of those but keep it in sequence.
    • try using sass variable.

    Happy coding :)

    Marked as helpful
    0