@PhoenixDev22
Posted
Hello @kelseychristensen,
I have some suggestions regarding your solution:
-
There should be two landmark components as children of the body element - a
main(which will be the component) and afooter(which will be the attribution).the<footer >should be outside the<main>. -
In this challenge , you can remove the extra div
<div class="parent ">and use:
<main class="card">
/* the rest of the code here.*/
</main>
- Anything with a hover style in a design means it's interactive. you need to add an interactive element
<a>around theimage ,Equilibrium #3429, Jules Wyvern. like this:
<h1><a href="#">Equilibrium #3429</a></h1>
<p class=""> Creation of <a href="#">Jules Wyvern</a></p>
-
Images must have an alternative text
image-equilibrium. You can useNFT name. -
For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images in (
icon-clock.svg, /icon-ethereum.svg", icon-view.svg) -
the
icon-viewdoesn't really need to be in the html , you could do it with css. If you want it to stay in html it needs to bearia-hidden:true ` orrole presentationwith empty alt -
the
icon-viewdoesn't really need to be in the html, you could do it with css. If you want it to stay in html it needs to be aria-hidden or role presentation with empty alt. -
Use <p> tag instaed of <header> .
<p>Our Equilibrium collection promotes balance and calm.</p> -
the link should be wrapping the original image and either have
Sr-onlytext, anaria-labeloralttext that says where that link takes you. -
You can use an unordered list
<ul>to wrapclass="info"and in each<li>, there would be<img >and<p>. -
No need for
<hr/>, you can useborder-topto the avatar's part. -
You should use
emandremunits .Bothemandremare flexible, Usingpxwon't allow the user to control the font size based on their needs. -
To center the card on the middle of the page , you can use the flexbox properties and
min-height: 100vhfor the<body>like this:
body {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
min-height: 100vh;
background: hsl(217, 54%, 11%);
font-family: 'Outfit', sans-serif;
}
-
an explicit width
width: 275px;is not a good way . Remove the width from the card component and change it tomax widthinstead. That will let it shrink a little when it needs to. -
You should use
emandremunits .Bothemandremare flexible, Usingpxwon't allow the user to control the font size based on their needs. -
About Table-Based Layout You should not use table-based layout under any circumstances .This design pattern is now considered very bad. It is bad for the user experience, bad for SEO, and bad for developers who have to maintain pages.Read more
-
try to put classes directly on anything you want to style.
Overall your solution is good , Hopefully this feedback helps.
Marked as helpful
@kelseychristensen
Posted
@PhoenixDev22 Thanks so much for your thoughtful and detailed feedback. I'm going to implement these changes :)
@kelseychristensen
Posted
@PhoenixDev22 I implemented a lot of these changes though I'm confused about the anchor tags around interactions. Including them means there are links that don't go anywhere... but maybe that's just because if this were a real, live site, the elements that have interactions would lead to a valid link. Thanks again for your thoughtful feedback.
