Any simplistic way to actually center the img in the container instead of playing with margin values ?
Brian Johnson
@BrianJ-27All comments
- @dratinixgithubSubmitted over 2 years ago@BrianJ-27Posted over 2 years ago
Kimue, hey!
So great job in coding this out, the design is pretty much spot on. To answer your question first, remove
margin-top, margin-bottom and margin-left
styles from your.card-container__img
class. Then change the width on that class from 250px to 100%. Next, on your.card-container
class, add the shorthandpadding
property of 10px to create even space around your qr image and that should do it my friend! Let me know if that works for you.Also there were a few things I noticed in your code that can remove the accessibility and HTML flags.
-
To remove the 1 accessibility flag on FEM, you need to have a level one heading (a.k.a) an
<h1>
tag in your project. The way you can fix this is simply replace your <h3> tag with an <h1> tag and reduce your font-size if need be to make it fit the design. -
Also
<article> & <section>
tags per web coding standards should have a heading tag nested within them. In your code, you have no heading tags nested within those tags which is why you are getting those 2 HTML issues. To fix that, change them to divs. Like this:
<section class="card-container"> to <div class="card-container"> <article class="card-container__img"> to <div class="card-container__img">
- Regarding using a background-image to bring in the qr image, it would of been better here to use an <img> tag. Using an image is good for a few reasons. One thing is it has more semantic meaning. Also if you want your images to be indexed by google use an img tag. Background images aren't indexed automatically. But you are not totally wrong to use it here. Just think about when it is appropriate to use them and when it is better to use img tags. Here's a good article on this: Img vs Background Images
Hopefully this helps and happy coding!
0 -
- @Louise-Ann93Submitted over 2 years ago
As my first frontend mentor challenge I thought i'd start small, a small component with just HTML and CSS. This was my experience;
Challenges I faced:
- I've definitely become accustom to the luxury that is VueJs and Tailwind CSS frameworks. It was like muscle memory to type in a tailwind class just to tweak some margin, in reality it was much back and forth between style sheets. I had to google more basic css than i'd like to admit.
- Importing the font was new to me, it's not something i've done before. Seems simple enough, in practice it didn't want to play ball at first.
- Being on one monitor, just my laptop it was quite difficult to look by eye if my sizing was completely right, too much or too little padding in places?
- At the end i worry i may have over complicated it a little.
Take aways;
- Great experience again with HTML and CSS, made me realise how comfortable i had gotten and not in a good way.
- Made me excited to take on more challenges.
Now i know there was no right way in completing this challenge but i would please appreciate someone taking a look and letting me know what i could have done either better or was there a way i could have simplified things? Over use in CSS that i maybe didn't need?
@BrianJ-27Posted over 2 years agoHi @Lou I think you did a good job here but I wanted to show you something. If you open up your inspector tool, you'll notice your content is not going to the bottom of the viewport page. This explains why your card is not perfectly center on the page. How do you fix that? Here's how:
- first remove all flex styles from the
<body>
tag. I know it sounds a little weird, but trust me on this. ;-) - Next you have a
<div>
tag that has no class associated to it. What I would do here is first change that<div>
tag to a<main>
tag for accessibility reasons, then cut theclass="container"
you have on the one div and paste it on the<main>
tag. Then rename that class from container to `class="card" because that's what it is. Let me visually show you what I mean.
<body> <main class="container> // changing your div tag to a main tag <div class="card"> // rename your class from container to card <img src="images/image-qr-code.png" alt="QRCode" class="qrcode"> <h1>Improve your front-end skills by building projects</h1> <p>...content </p> </div> </main> </body>
-
Now on the
class="container"
addmin-height: 100vh
. What this does is, makes your container content area go all the way to the bottom of the page. Next you add back those flex properties,display: flex
justify-content: center
align-items: center
. Once you write those flex styles, this will perfectly center your card on the page! you can then remove themargin-top: 100px
style bc you won't need it. -
another thing you want to think about is using rem units in lieu of pixels. Pixels are fixed units and don't play well with making responsive layouts so try to think about this for future projects.
-
on your img styles, you don't want to use fixed pixel widths because your image won't scale. On the width I would use
max-width: 100%
in lieu of width.
Sorry I wrote a lot of things to change but try to implement these things and see if it helps you :)
Marked as helpful5 - @PeallyzSubmitted over 2 years ago
It's my best attempt but it's not perfect. I have some issues on mobile screen with rating box behind the comment. I'm not sure that the comment card are done by the best way.
That was cool to do that challenge.
@BrianJ-27Posted over 2 years agoHey Peally,
Good job with getting this project completed. I do have some suggestions that can can help you.
-
Please take a deep look at your basic HTML structure. It is incorrect. especially your
<section>
tag that has your<body>
tag nested within it. Please remove that section tag from your code. Here's a link that will walk you through how to properly setup a basic HTML boilerplate structure. [https://www.sitepoint.com/a-basic-html5-template/] Refactor your html structure and you will lose a lot of these HTML issues above. -
In your code you are not correctly using
<section>
tags. When you use them, you have to have heading tags nested within them like an<h2>
tag for example. So I would simply change your<section>
tags to<div>
tags. -
you must always have one main landmark or
<main>
tag on the page for accessibility reasons. so in your case, change your<section class="top">
to<main class="top">
and the closing tag too..
So those are a few things to start but overall good job my friend
Marked as helpful0 -
- @kpax10Submitted over 2 years ago
I'm fairly happy with the way this turned out. Please feel free to give any suggestions!
@BrianJ-27Posted over 2 years agoHey Kpax
Overall the layout looks really good visually. You matched the box shadow perfectly on the cards and I may have to steal that color from yours and add it to mine! :) Also great job in not getting any accessibility and html errors. When I checked out your code, there a few things I noticed:
-
Avoid using fixed widths and try using more flexible widths like % or rems. "ex) width 1100px" The only time I would want to used a fixed width is if it was for something intentional. So for example, I only want my card to be a maximum width of 400px so in this case, instead of using width, I would use, "max-width: 400px". This is better and more flexible because your card's width will adjust to the screen until it reaches 400px and then it won't grow no more. Also any where you are using px's see if you can't convert to rems unless you intentionally need it to be px's.
-
I see you are using Sass which is awesome and I noticed you are still using the @import directive. Very soon this will be deprecated and be replaced with @use & @forward. If you need some help in implementing it, on Youtube, Kevin Powell provides a great breakdown on how to do this. Here's the link. https://www.youtube.com/watch?v=CR-a8upNjJ0&t=10s
-
You have unused styles in your style.css. To be more specific, on your .card-container class on lines 119-126 you don't need, flex-direction: row (on line 121) because display: flex will default to a row direction.. Also you don't need "margin: auto & width: 1140px" (on lines 124 & 126) because you already defined "justify-content: center" which is centering your content horizontally, so now we just cleaned up your css a bit. :) So my suggestion would be to take a moment and refactor your code and see if you can make it more cleaner.
But overall great job
Marked as helpful0 -
- @Johnselastin7854Submitted over 2 years ago
All feedbacks are welcome
@BrianJ-27Posted over 2 years agoGreat Job overall.
I checked out your code and visually it looks good. If you want more specific feedback, I like to try to match the design even down to how many lines of content are there. In your solution, there are 2 lines for your content and the design shows it breaks to a third line. I would maybe increase your font-size a bit or put a little padding on each side of your p tags.
Also to fix your accessibility issue:
- change your div tag to a main tag (or main landmark)
- change your h3 to an h1 (each page should have at only 1 h1 tag per page)
This should clear up your accessibility flags but overall nice job and happy coding!
0 - @dominikapapSubmitted over 2 years ago
-Did I use any bad practice? -Is using px ok here, or should I have changed them to rem? -What could have been done better?
@BrianJ-27Posted over 2 years agoHey There @dominikapap Great job overall laying out this project. You have this card square in the middle and that is awesome!
I have a few things to share with you regarding your codebase. I'll just number them for you :)
-
for your <section class="main"> tag, change the <section> tag to a <main> tag
-
for your <section class="footer"> tag change the <section> tag to a <footer> tag **Formatting your code this way should remove all of your accessibility & HTML issues with the code
Regarding best practices, I see you have 2 css stylesheets in the root directory of your project. It is usually best to create a "css folder" then place those 2 stylesheets within that folder. When you do that, you next need to go back to your index.html file and modify your filepath to the stylesheets.
That's all I see for now at quick glance but great job overall. Happy Coding
Marked as helpful0 -
- @BrianJ-27Submitted almost 3 years ago
Any feedback and thoughts about my code are welcome! I am always looking on better ways to make my code more DRY and understandable
@BrianJ-27Posted almost 3 years agoHi Naveen! Thanks for your feedback. So yes I already added the main tag but what I didn't add when I first published this project was the h1 tag (which I believe caused the accessibility issue). I went back and updated the code to include that but it didn't refresh and take away the accessibility flag.
0