Skip to content
OverviewSolutions
18
Comments
53
Rohit Deshpande
@rohitd99

All comments

  • Benevolent•490
    @bene-volentSubmitted over 1 year ago

    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.

    Profile Card Component with BEM

    1
    Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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

    1
  • kudos2Shef•170
    @kudos2ShefSubmitted over 1 year ago

    Hi, My second challenge so far. Curious to know how to write efficient HTML/CSS code. Feedback are welcome!

    HTML CSS FLEXBOX

    3
    Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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

    1
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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.

    1
  • Jan van Ierssel•20
    @JanvampiersselSubmitted over 1 year ago

    Ran into some sizing issues that took me way too long to solve, but at last....

    NFT Card Design

    4
    Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    2
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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

    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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.

    1
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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

    0
  • Rohit Deshpande•870
    @rohitd99Posted over 1 year ago

    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
    0