@pikapikamart
Posted
Hey, awesome work on this one. Layout in general looks great and it is responsive.
It is great that lots of other already gave their feedback on this, really really nice. Just going to add some suggestions as well:
- You don't need
margin
to center the content since it is not consistent right now for all users in terms of being centered. What you can do is that first removemargin
on themain
and on thebody
tag add:
align-items: center;
display: flex;
flex-direction: column;
justify-content: center;
min-height: 100vh;
This way it will be centered properly.
- Those decorative images on the site could have use an extra
aria-hidden="true"
attribute so that they will be totally hidden. - Music icon is decorative only so hide it using the method I mentioned.
- Also when using
alt
attribute, avoid using words that relates to "graphic" such as "logo" and others. Animg
is already an image/graphic so no need to describe it as one. annual plan
should be a heading tag since it gives overview on what the section contains.- Also, when there is a text-content, always use meaningful element to wrap those and not only just
span
.
Aside from those, great work again on this one.
Marked as helpful
@peter-abah
Posted
@pikamart Thanks. I didn't use flex on the body element because I thought it was wrong is body is like the root element. I'll read up on the aria hidden attribute
@pikapikamart
Posted
@peter-abah Hey, for components like you can since there are no other content that is being used. But if you don't like it, maybe just use the main
as the selector to be styled and just add another div
inside it that will wrap the main content.