P
İsmail Hasır• 60
@ismailhasir
Submitted
artemkotko14
@artemkotko14
All comments
- artemkotko14• 30
@artemkotko14
Posted
Great job on this challange! A few things I would suggest to consider:
- 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.
- Your buttons don't have a focus state, so I can't move through your page without a mouse.
- Add
cursor: pointer;to your hover state, to make it look the same as at the design. - Also you used px for your font-size which is not a good practise. You can read aboit it in this article: Article
0 - VarunOffical• 100
@VarunOffical
Submitted
artemkotko14• 30@artemkotko14
Posted
- 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.
- 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.
- 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.
- 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
- Font size shouldn't ever be in pixels. Use rem instead.
- Your h1 shouldn't be centered. Check again at the original design.
- I don't see
font-weightandfont-sizeproperties on most of your elements. - Your button is missing a
border-radius
0 - Jennifer• 70
@x3blondie
Submitted
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
artemkotko14• 30@artemkotko14
Posted
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
