Design comparison
Solution retrospective
During this challenge, I initially used the <article> tag to build the card in a semantic way, but someone told me that using the <div> tag. I also struggled with picking px amounts when using properties like margin and padding, and positioning elements. In the future, how can I improve on these two issues?
Community feedback
- @AdrianoEscarabotePosted about 2 years ago
Hi Christine Belzie, how are you?
I really liked the result of your project, but I have some tips that I think you will enjoy:
As you know this project is based only on one page component, so no h1 is needed! because we don't know if this page will have a more important component, but it's always good to prevent accessibility errors so I think it would be good for you to add an
h1
in this component, besides being a good practice for when you are developing larger sites, don't worry forget abouth1
.To centralize your prefer to do this:
body { margin: 0; padding: 0; display: flex; align-items: center; flex-direction: column; justify-content: center; min-height: 100vh; }
The rest is great!
I hope it helps... 👍
Marked as helpful1 - @grace-snowPosted about 2 years ago
Hi
I think this needs some more work as the html structure isn't quite right and it doesn't yet look like the design when I'm viewing on mobile (I'll add a screenshot to slack for you)
Html
- you don't need to wrap everything in a div
- only use headings when they are actually headings for other content. London is not a heading, for example
- think about how the statistics (stats) would be written in a plain unstyled document. These each have to be read together, number and word, to make sense. "80k followers" not "80k" and then a new paragraph. That indicates this needs to be a list with 3 list items, one for each stat. You can wrap the number in a strong tag or span and style as display block so it sits on its own line in the card
- the attribution needs to be a footer element. Landmarks shouldn't be nested at different levels. Remember you can place classes directly on main / footer you don't always need to nest extra divs unless it's really needed
- Indent your code consistently! This is Really hard to read at the moment. Your code editor can even do this formatting automatically for you
- decorative images like the pattern at top of the card should have empty alt
- you cannot have a header in a card and a header cannot contain only decorative (effectively no) content
Css
- again, format. Give the styles some.space to breathe with space between each declaration
- the card should not have a width or height only a max-width
- give the card a little margin on all sides
- I don't think the background circles can be positioned correctly using px values. This is really tricky actually and I ended up using pseudo elements if you want to take a look at my solution
- not sure why the card is position relative
- font size must never ever be in px. I can't stress this enough. Use rem. Extremely important for an accessible site
- never style on IDs. That's not what they are for and increases css specificity for no reason. Prone to bugs in larger projects as developers wouldn't expect changing ID to affect styling
- use colors from the style guide not key words
2 - @BilalSalmiPosted about 2 years ago
Hi Christine, good effort!
Your design needs some optimization which are as follows:
- consider to use alt attribute in your <img> tag .
read more about alt attribute and html images here.
- consider to use <h1> tag in your page.
Read more about html heading here.
- to center your card consider to use css height in your html body
for example :
height: 100vh
. it will perfectly center itkeep it up!
0@grace-snowPosted about 2 years ago@BilalSalmi use min height 100vh not height. It's an important difference
0
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord