@pikapikamart
Posted
Hey, really nice work on this one. The overall layout looks great.
Here are some suggestions for the site:
- For this one or just in general, when you building a site, always have a single
main
tag to wrap the main-content of that page. For this one, the.main
should be using amain
tag instead ofdiv
so that the site will contain a main-landmark. - Those 2 blobs images could be used as
background
value. This way, you won't have to create those 2div
. - For the
div
that wraps the.attribution
, usefooter
tag on it so that it will be nested inside in another landmark. So on this page, you would have a markup that looks something like this:
<main />
<footer />
- Remove the
position: absolute
on the.main
since it is not needed, this just hides the.attribution
as well. Remove theposition: relative
from thediv
that wraps the.attribution
. For this, you don't really need them. So you will have to remove the stylings:
position
left
bottom
Since the .App
is using a flexbox already to center both items.
- Also on the
.App
, remove theheight: 100vh
. Avoid usingheight: 100vh
on a large container as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - For this one, since the layout is showing a person's information and statistics, it makes sense to use the person's name as the person's
img
alt
value. - You could use for now the
h1
to wrap the person's name since a singleh1
is needed for a site. If you feel likeh1
is not suited on this, useh2
on the name and have a search aboutsr-only h1
. - When wrapping up a text-content, make sure that it is inside a meaningful element like
p
tag or heading tag and not using likediv, span
to wrap the text. - For the
.stats
selector, if you look at the content of that part, those are "list" of information about the user, it would make sense to use aul
ordl
tag in that one.
Aside from those, great job again on this one.
Marked as helpful