@pikapikamart
Posted
Hey, awesome work on this one. Layout in general looks great.
Others already gave their feedback on this one, just going to add some suggestions as well:
- Right now, if you hover on the
html
tag orbody
, you will notice that it has no height since the main component is usingposition: fixed
which takes an element out of the flow. Avoid using this orposition: absolute
. Since you are using this to only center the content, it will be better to do it this way, but first, remove all these properties on the.card
selector:
left
top
position
transform
and on the body
tag add these:
align-items: center;
display: flex;
justify-content: center;
min-height: 100vh; # so that flexbox will have enough height to center vertically
This is more consistent as this will always center the content and you avoided using position
property on the layout.
- Vector image should be hidden since it is only decoration. Decorative image must be hidden at all times for screen-reader users by using
alt=""
and extraaria-hidden="true"
attribute on theimg
tag. - Also when using
alt
attribute, avoid using words that relates to "graphic" such as "background" and others. Animg
is already an image/graphic so no need to describe it as one. - A page must have a single
h1
on a page. Since there are no text-content that are visible that could beh1
, you will make theh1
screen-reader only text. Meaning this will be hidden for sighted users and only be visible for screen-reader users, search aboutsr-only
stylings and see how it is used. Theh1
text should describe what is the main content is all about, thish1
would be placed as the first text-content inside themain
element.Have a look at this simple snippet of mine implementing the sr-only heading tag. I already added some comments in the markup so that it will be easy to understand^^. - Music-icon should be hidden as well using the method I mentioned.
- When wrapping a text-content do not just use
span
to wrap it, use meaningful element like ap
tag if it just a regular text or heading tag if it is an heading. annual-plan
could use a heading tag since it gives information on what the section would/could contain, hence the pricing for such plan the user has chosen.
Aside from those, great job again on this one. If you have any queries just let me know.
Marked as helpful
@GitNutts
Posted
@pikamart Appreciate the detailed response. I've now updated my code.
Can you explain why it's not ideal to use position
?
Thanks
@pikapikamart
Posted
@GitNutts Hey. Well not idea if you are going to use position: absolute
or positiont: fixed
on a large element.
Because like what I suggested above, if you inspect the layout, you don't have any height because the main content is out of the flow because of position: fixed
.
You can use those position for example, a navigational dropdown right, you will need to use position: absolute
or position: fixed
for those.
Marked as helpful