Skip to content
OverviewSolutions
10
Comments
10
Artem Kotko
@artemkotko14

All comments

  • Artem Kotko•190
    @artemkotko14Posted about 1 month ago

    Great job with completing this challange!!! I like your design and your responsiveness is also great.

    A few minor suggestions:

    1. You can add focus states to your buttons for better UI.
    2. Also you forgot to change a color for a button when it's on hover.
    3. I can also recommend you to use hgroup element for better semantic htmnl instead of plain divs here:
            <hgroup>
          <p>Report for</p>
          <h2>Jeremy Robson</h2>
        </hgroup>

    Keep on coding!

    Marked as helpful
    0
  • Artem Kotko•190
    @artemkotko14Posted about 2 months ago

    Congratulations on completing this challenge!

    You've done really impressive work here. The only minor adjustment that I can recommend you is adding a focus state to your buttons to improve UI.

    Keep on coding!

    0
  • Artem Kotko•190
    @artemkotko14Posted 2 months ago

    Congrats on completing this challange!

    You have chosen very creative way to complete it. Some of your code is hard to understand for me, for example what is button (withjs/withcss) is doing.

    Just a few things that I want to point out:

    • Your design got a lot inconsistencies with the original design. Image, font-size, etc. There are lots of things that you can improve.
    • Font-size should never be in px. Use rem instead.
    • You have used px everywhere which is not a great practice. You can read about it here.
    • Your final result still got some bugs. Just some that I cought:
    1. When I resize the page manually from desktop to mobile size with share div visible, it will show me two divs in mobile size. It shows like this.
    2. If I switch between withcss and withjs with share in active state and click on a button again it shows two same divs. Like this

    Hopefully my feedback will be somewhat helpful for you. Cheers!

    0
  • Oscar Rodolfo Chávez•170
    @OscarRodolfoUMGSubmitted 4 months ago
    What are you most proud of, and what would you do differently next time?

    Ok, I feel like I needed some advice that I last read in a feedback, but I'm happy that the grid worked well and like sometimes you already know how to correct small things.

    What challenges did you encounter, and how did you overcome them?

    I feel like a challenge is adapting to multiple resolutions, because there has to be a scaling between the different resolutions in the media queries, but in the end I managed, somehow, to make it look presentable.

    What specific areas of your project would you like help with?

    On how to organize the media queries, which was what got me behind in the end.

    Testimonials grid section [HTML & SASS]

    #sass/scss
    2
    Artem Kotko•190
    @artemkotko14Posted 4 months ago

    Great job on completing the challange!

    Couple of things I would suggest:

    1. Your media query is too small (min-width: 414px). I would recommend to at least make it 700px. You can resize your page in your browser and you will see that on lots of sizes of the page not all of your content can fit to the page. Another option would be to use different media queries with different min-widths and different grid-areas.

    2. Also your cards have fixed width and height which is also not great. Instead of width: 70rem; I would recommend you to use width: 100%; and height: 100%; on every card and just give some max-width. This will ensure cards fill the grid column widths and heights.

    Keep on coding!

    0
  • Artem Kotko•190
    @artemkotko14Posted 4 months ago

    Congrats, you have done a great job! I can only suggest a couple of changes to your code:

    1. Specify alt on your images. This text helps screen-reading tools describe images to visually impaired readers and allows search engines to better crawl and rank your website. So it's a good habbit to always include that.
    2. You are using 90rem for your media query. On a full size of my laptop screen I can still see a mobile design. I would suggest to use something between 40-60 rem. Hope this will help. Keep on coding!!!
    Marked as helpful
    0
  • Artem Kotko•190
    @artemkotko14Posted 4 months ago

    Your desktop design looks great. However the image stays the same when switch to a small screen. To switch your image based on the screen size I would advice you to use picture element. Also, I would not suggest to use h1 for a small ribbon text above the name of the product. h1 must indicate the primary topic of your webpage. hgroup element with heading and paragraph Parfume for this would be much better practice. Hope this will be helpful for you. Keep on coding!

    Marked as helpful
    1
  • Artem Kotko•190
    @artemkotko14Posted 5 months ago

    I have nothing to say, except very well done!!! Keep on doing your great job.

    0
  • Artem Kotko•190
    @artemkotko14Posted 5 months ago

    Great job on this challange! A few things I would suggest to consider:

    1. Use <a> or <button> instead of <li>. Think of it as a real website, if a user clicks one of these buttons, it takes them to another page. With <li> you are not gonna have possibility to implement such functionality.
    2. Your buttons don't have a focus state, so I can't move through your page without a mouse.
    3. Add cursor: pointer; to your hover state, to make it look the same as at the design.
    4. Also you used px for your font-size which is not a good practise. You can read aboit it in this article: Article
    0
  • Artem Kotko•190
    @artemkotko14Posted 5 months ago
    1. Get into the habit of including a full modern css reset at the start of the styles in every project. Andy Bell or Josh Comeau both have good ones you can look up and use.
    2. This solution currently overflows my screen at the top so there's some content cut off that I can't reach. This card mustn't have a height. That's important to understand that you shouldn't limit the height of elements that contain text. Let the browser do it's job and decide what height is needed based on the content and spacings inside. As soon as you limit the height the card has the potential to break, like if an author changes the amount of text in there or a user changes their default text size or translates the site into another language.
    3. The card shouldn't have a width either. Give it a max width in rem instead. That way, it can shrink narrower if it ever needs to like on a small screen.
    4. The card doesn't have hover or focus states, which was the main challange here. If you need help with that you can read more about it here:
    • focus state
    • [hover](https://www.w3schools.com/cssref/sel_hover.php
    1. Font size shouldn't ever be in pixels. Use rem instead.
    2. Your h1 shouldn't be centered. Check again at the original design.
    3. I don't see font-weight and font-size properties on most of your elements.
    4. Your button is missing a border-radius
    0
  • Jennifer•70
    @x3blondieSubmitted 6 months ago
    What are you most proud of, and what would you do differently next time?

    Centering divs

    What specific areas of your project would you like help with?

    How can i better document in github

    qr-code-component

    2
    Artem Kotko•190
    @artemkotko14Posted 6 months ago

    Great job!!!

    I would suggest to put your <div class="attribution"> at the bottom of the page. You can do so with { position: fixed; bottom: 0; } or some other ways that you prefer. Don't forget in CSS file we use /* */ for comments. Also try to experiment with sizes as your container looks bigger then it is on an original design. But overall I like how your code is well-structured and readable!

    0