@andreasremdt
Posted
Hey @Toxgem,
Nice work on this challenge! It looks quite close to the design and has no HTML or accessibility issues, which is great!
Here are a couple of possible improvements I can give you:
- Using the below code, you can easily align your card in the horizontal and vertical center of your page.
min-height
is used to make the page as big as your browser's window, otherwise thebody
will only grow as much as the content inside requires it to.
body {
display: grid;
place-items: center;
min-height: 100vh;
}
- The way you solved the image hover effect is quite creative and interesting, but there are better ways. Try looking into CSS pseudo-elements, like
::before
and::after
. They allow you to style HTML elements that are not really in your markup, but are still there for you to use. It's a bit tricky to explain in text, so you can check out my solution to see how I solved it. - You can try to reduce the amount of HTML used. For example, your
h1
contains aspan
, which in return has a CSS class for color. You can remove thespan
and apply the class directly on theh1
. The same applies to the below paragraph, which is inside adiv
without any styling. You can safely remove thediv
, as it doesn't do anything. - Using custom margins for spacing is the way to go, I don't see anything wrong with how you did it :-)
- I would also go with
rem
in most cases. However, I use other units depending on what I am trying to style, e.g.px
for border widths. Kevin Powell has a great video explaining when to use which unit, maybe this clears things up for you. https://www.youtube.com/watch?v=N5wpD9Ov_To - For the Ethereum and clock icon, you used
img
elements. While this works, it's not optimal in terms of semantic meaning.img
elements should be used whenever you want to show an image that's relevant for the content, like a picture of a car on a car sharing site. These two graphics, however, are more for styling purposes and to make the layout look good, so it's better to use CSSbackground-image
for that and remove them from the HTML. If you want to keep usingimg
elements, make sure to applyaria-hidden="true"
to them so that screen readers don't think that this is an important piece of content. Also, you can remove the alt text by setting it toalt=""
. Alt texts should only be present on images that have a meaning to the content.
Hope that my tips made things clearer for you, if you have any questions feel free to drop them in the comments :-)
Marked as helpful
@Toxgem
Posted
@andreasremdt Thanks you for taking the time and reviewing it, i'll try to implement it all