Skip to content
OverviewSolutions
18
Comments
53
Rohit Deshpande
@rohitd99

All comments

  • Rohit Deshpande•870
    @rohitd99

    Hi Benevolent

    Congrats on completing the challenge.

    I'd like to suggest a few changes to your solution,

    1. For the attribution 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.
    2. I also find there is no heading in your page, each page should generally contain a h1 for the title, so I think the name of the person can be a h1 instead of p for the title.
    3. The image of the the person is not a decorative image so it should include a 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

  • Rohit Deshpande•870
    @rohitd99

    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
  • Rohit Deshpande•870
    @rohitd99

    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.

    1. You've used div's for structuring but I'd suggest using main and section as they are semantic elements.
    2. For representing the deleted text use del instead of p.
    3. The spelling of Perfume is mistaken as Prefume.
    4. Also instead of having two images and changing them through media queries, I'd like to suggest using picture element with two different source's. Here's the MDN doc for the same picture.

    Hope it helps

  • Rohit Deshpande•870
    @rohitd99

    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.

  • Rohit Deshpande•870
    @rohitd99

    Hi Jan van Ierssel

    Congrats on completing the challenge.

    I just wanted to point out a few things, which can improve the solution.

    1. I noticed you've used 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.
    2. Also on you've put 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
  • Rohit Deshpande•870
    @rohitd99

    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
  • Rohit Deshpande•870
    @rohitd99

    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 divs. Overall a very neat and clean solution.

    Hope it helps

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    Hello Rohit, from Rohit 😁

    Congrats for completing the challenge.

    I have but a few suggestions for you.

    1. You have used 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.
    2. Secondly I see you've used 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.Video

    Hope it helps

  • Rohit Deshpande•870
    @rohitd99

    Hi YUVASRI06

    Congrats on completing the challenge.

    I'd like to suggest a few changes for improving your solution.

    1. You've used a 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.
    2. Instead of simply using a div to wrap your content , you can use semantic elements like main, section as per need.
    3. Add 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.
    4. Also for your image you can have width: 100% instead of a fixed width.
    5. Use max-width on your container so that it can lower beyond a certain size.
    6. I also noticed a single p tag which might have been placed mistakenly so you might want to remove that too.

    Hope it helps

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    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.

  • Rohit Deshpande•870
    @rohitd99

    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

  • Rohit Deshpande•870
    @rohitd99

    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
  • Rohit Deshpande•870
    @rohitd99

    Hi Jouter

    Congrats on completing the challenge.

    Firstly I don't think you need the media query , you can have this property on the "QRCodebox" which is max-width: 18rem and it will scale down if needed on smaller screens, if you want to scale for larger screens this article here is a good read. I also noticed you've used h2 for the title , it should be h1 you can style it as per need . Each page must have a single h1 and all headings must be in order from h1 to h6. Also I'd suggest using semantic elements like main,section than just plain old div.

    Hope it helps

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    Hi Natalie

    Congrats on completing the challenge.

    I have a few suggestions for the same

    1. You have height : 100vh on your body , it is better to have min-height: 100vh since it allows the content to expand if it is greater than 100vh. Also add some padding to your body so that on smaller screens the content doesn't cover the entire screen and there is some space left.
    2. You also have width property set on the div class="card" , again max-width would be better suited since it allows to scale down. Similiarly for the img on mobile you can have width: 100% for it (as it covers entire width of card) and for larger sizes have max-width: 1100px and below it width: 100% so that it remains responsive on those sizes.
    3. The usage of h1 for the title is correct, but then you've again used it in your stats div. Every page must have only a single h1 and headings must always be in order from h1 through h6. So maybe h2 would better suited, but I feel that those stats arent' headings of sort and a p may also be fine. I'll leave that to personal choice.

    Hope it helps.

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    Hi StudentForEternity

    Really cool solution first of all. I have only a single suggestion you have width: 320px on your div with main-grid class. I'd suggest using max-width: 320px instead so it can responsive for widths below that although there are not many screens below 320px.

    Hope it helps

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    Hi Adéla

    Congrats on completing the challenge

    I have a few things to add, which might help you

    1. You have a height: 100vh on your body, I'd suggest using a min-height : 100vh instead since the former stops the content at 100vh and doesn't let it grow further while the latter allows the expansion as per content.
    2. The card has a width : 320px , I think a max-width : 320px is better since it allows the content to minimize on smaller screens (although there aren't many smaller than 320px). On the image too you have width and height directly on the element, you should put them in the stylesheet instead. Again I'd recommend using the width:100% on the img and let the height adjust itself than the fixed numbers. If you have fixed numbers they are rigid/fixed and not responsive.
    3. For font-sizes use rem than px. Reason why is linked here
    4. Lastly I'd suggest using semantic elements like main, section etc than just plain old div.

    Hope it helps

  • Rohit Deshpande•870
    @rohitd99

    Hi Othman

    Congrats on completing the challenge.

    Firstly you have kept the title as a h2 heading but it must really be a h1 heading. Each page must have a single h1 heading signifying the page title. Also headings must always be from h1 through h6 in order. These are mainly for semantic reasons and SEO. Secondly the header image can be a background image since it has no specific meaning. Other than these a really nice solution.

    Hope it helps

    Marked as helpful
  • Rohit Deshpande•870
    @rohitd99

    Hi beqa-burduli

    Congrats on completing the challenge.

    I have a few suggestions for you.

    1. You've used the div class="main_conteiner" ,I'd recommend using the main element instead since it is semantic element.
    2. h3 has been used for the title. I believe you've used it for it's font size, but you should rather use it as per it's meaning. Headings must always be from h1 through h6 in order. Also for each page there must always be a single h1 heading signifying the page title. Thus I think h1 would be more fit for it.
    3. Add the alt attribute for the NFT image since it is not a decorative image.

    Overall it was a nice solution. Hope it helps