@bene-volent
Submitted
I could think of how to put background image without using html as a container. Then @abraund posted his solution and I took the inspiration from his summary of the solution.
@rohitd99
@bene-volent
Submitted
I could think of how to put background image without using html as a container. Then @abraund posted his solution and I took the inspiration from his summary of the solution.
@rohitd99
Posted
Hi Benevolent
Congrats on completing the challenge.
I'd like to suggest a few changes to your solution,
footer
would be better suited than aside
, aside
is generally used for indirectly related content and footer
is used at the end for information about author, links, contact details etc.h1
for the title, so I think the name of the person can be a h1
instead of p
for the title.alt
attribute to describe it for assistive technologies or when it fails to load there must be something to describe the image.Hope it helps
@kudos2Shef
Submitted
Hi, My second challenge so far. Curious to know how to write efficient HTML/CSS code. Feedback are welcome!
@rohitd99
Posted
Hi kudos2Shef
Congrats on completing the challenge.
I noticed that to center the card you've used properties like position : relative
etc on your card, well to center something you don't need these but simply use a flex or grid on main
main {
min-height: 100vh;
display: flex;
flex-direction: column;
justify-content: space-around;
align-items: center;
}
add these to your solution and remove the position : relative
and top : 120px
from your card. Also I see you've used headings in the wrong way. The card title must he a h1
instead of an h3
. Each page must have a single h1
heading for the title. Headings must be in order from h1
through h6
. Same for the footer instead of h6
, I think semantically a p
element should suffice as I don't think that is a heading.
Hope it helps
Marked as helpful
@rohitd99
Posted
Hi Utkarsh
Congrats on completing the challenge.
Enjoyed going through it, well structured, and responsive. But I have a few suggestions that will help you improve it.
div
's for structuring but I'd suggest using main
and section
as they are semantic elements.del
instead of p
.picture
element with two different source
's. Here's the MDN doc for the same picture.Hope it helps
@David23-Dev
Submitted
My English is bad sorry, this is my first challenge and I just started learning html and css
@rohitd99
Posted
Hi David
Congrats on completing your first challenge 🥳🎉.
Also no need to apologise for your english. Now coming to the challenge I have some suggestions for you.
I see you've implemented the solution nicely for the mobile and desktop sizes but for tablet size they don't cover the entire width.
@media screen and (max-width: 600px)
{
body{
justify-content : start;
}
.contenedor {
box-shadow: none;
flex-direction: column;
height: auto;
/* width: auto; */
width: 100%;
justify-content: start;
/* align-items: center; */
}
#tusResultados {
border-top-left-radius: 0;
border-top-right-radius: 0;
width: auto;
}
#summarry {
width: auto;
}
}
I believe these properties should make it better on tablet sizes.
Other than that I also see you've used headings in a wrong way, h1
heading must only be used once per page. All the headings must be in order that is h1
then h2
and so on. Here's a video for correct usage of headings.
Overall you've done well for your first solution, used semantic elements. Keep going and you'll learn things in no time.
@Janvampierssel
Submitted
Ran into some sizing issues that took me way too long to solve, but at last....
@rohitd99
Posted
Hi Jan van Ierssel
Congrats on completing the challenge.
I just wanted to point out a few things, which can improve the solution.
h3
for the title, but an h1
heading would be more suited. For each page a single h1
heading is always expected and all the headings must be in order from h1
through h6
. h1
generally goes for the title, h2
for subtitle and so on.height: 100vh
on body , main but min-height : 100vh
should be used if you want them to stretch as per content. Although on this challenge it wouldn't make much difference.Hope it helps
Marked as helpful
@alghaylan
Submitted
@rohitd99
Posted
Hi abd nour
Just a small suggestion, I saw that you've used a h3
heading for the title , instead use an h1
heading. Each page must have a single h1
and all the other headings must be in order that is from h1
through h6
. They carry meaning and <h1>
defines the most important heading, and <h6>
defines the least important heading. Thus h1
should be the most suited for the title.
Hope it helped
Marked as helpful
@wolfgunblood
Submitted
Any comments regarding this challenge ??
@rohitd99
Posted
Hi wolfgunblood
Congrats on completing the challenge.
Really cool solution, Firstly I see you've used h2
for the title I'd recommend using h1
instead and styling it as required. There must always be a single h1
heading on a page and all headings must be in order that is from h1
to h6
. Secondly I'd suggest using semantic elements like main
, article
,section
instead of just plain old div
s. Overall a very neat and clean solution.
Hope it helps
Marked as helpful
@RohitPatra-2002
Submitted
@rohitd99
Posted
Hello Rohit, from Rohit 😁
Congrats for completing the challenge.
I have but a few suggestions for you.
article
as semantic element correctly so I'd recommend using main
too instead of the div class="container"
since the card is the main content of this page.div
for the background image I'd say you can use background-image
property on your body for the same, since those images don't have any meaning this should be better. Here's a video from Kevin Powell that shows you how to use multiple images as background.VideoHope it helps
@YUVASRI06
Submitted
@rohitd99
Posted
Hi YUVASRI06
Congrats on completing the challenge.
I'd like to suggest a few changes for improving your solution.
h3
element for the title I'd suggest using h1
since each page must have a single h1
heading and all headings must be in order from h1
through h6
.div
to wrap your content , you can use semantic elements like main
, section
as per need.min-height : 100vh
instead of just height: 100vh
since that blocks the content from expanding if height is larger than 100vh , although on this challenge it is certainly not an issue but on challenges which expand more than the screen size it can be one.width: 100%
instead of a fixed width.max-width
on your container so that it can lower beyond a certain size.p
tag which might have been placed mistakenly so you might want to remove that too.Hope it helps
Marked as helpful
@saeedahmedasad
Submitted
@rohitd99
Posted
Hi Saeed Ahmed
Congrats on completing the challenge 🥳.
I see that you've used your headings in an incorrect way. Generally headings are used from h1
through h6
and each page must only have a single h1
for the title. So I would suggest you the h1
for the "Your Result", and h2
for the "Great" and "Summary". Secondly you've used the height: 100vh
on your body and the main in media query, I would suggest using min-height : 100vh
so that the minimum height is 100vh and it can expand further as per content.
Hope it helps.
@Ragu-The-Developer
Submitted
Hey There I Have Completed This Project But Backgrounds And Other stuffs like profile merge in background along with footer contents were little messy.
@rohitd99
Posted
Hi Ragu-The-Developer
Congrats on completing the challenge 🎉.
I've found that you have used multiple h1
mainly for the name, and in the footer. Generally for each page there must only be a single h1
heading and the headings must always be in order from h1
through h6
in order. I would suggest using h1
for the name but for the footer-items p
element should suffice. Secondly you've used the b
element , I'm not sure if it is deprecated but I think that you can add font-weight
in the style and use a p
element instead. Also if you want the image to stay on top of the top-background of the container as in the challenge you can add the following properties:
.profile {
position: relative;
bottom: 50px;
display: flex;
justify-content: center;
border-radius: 50%;
border: 5px solid white;
margin-left: 35%;
}
Hope it helps
@AshwinKumar0
Submitted
@rohitd99
Posted
Hi @mRkOoL
Congrats on completing the challenge 🎉.
I have a few suggestions which might help you. You can use the following properties on your body to keep the container centered:
min-height: 100vh;
display : flex;
justify-content: center;
align-items: center;
The min-height: 100vh makes sure the height is atleast 100vh and can expand further if the content allows. Also remove the margins on the container for it to be exactly centered. In your media query for the container you can have the following properties as on tablet sizes I noticed the content was overflowing the screen.
media screen and (min-width: 500px)
.Container {
flex-direction: row;
max-width: 700px;
width: 100%;
/* min-width: 600px; */
height: 350px;
/* margin-top: 100px; */
}
You also don't have an h1
, generally each page must have a single h1
for the title of the page. Although I agree there isn't any text to signify a title you can create a title which is only visible for screen readers. You can have a class sr-only
which hides the text visually but not from screen readers and also keeps the h1
in the page for SEO reasons.
.sr-only {
border: 0;
clip: rect(0 0 0 0);
height: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute;
width: 1px;
}
Hope it helps.
Marked as helpful