Skip to content
OverviewSolutions
14
Comments
20

Joyal James

@silkcoder265 points

Freelance web developer

Latest solutions

View more solutions

Latest comments

  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Congrats for completing the task.

    I noticed one thing, You applied the style height:100vh to the container element. Its good but min-height:100vh is more suitable here.

    To see why, open your site on google chrome, then open developer tools, then click on mobile view, then choose Iphone 12 pro, the site looks good but if you change the orientation, you will see some white colored space at bottom. You can fix this by applyingmin-height:100vh instead of height:100vh to the element with class 'contianer'.

    Good luck with your future tasks.

    Marked as helpful
    1
  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Congrats for completing the task.

    I noticed one thing, you applied flex properties to the HTML tag to center the 'card' class element.I think you should apply it to the body tag rather than applying to the HTML tag.

    It's generally considered a good practice to put styles in the <body> element

    1
  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Good attempt.

    You can place your container in the center of the screen vertically by changing the height property of the .container class

    change min height:600px to min-height:100vh

    .container {
   min-height:100vh
}

    Now the container get placed vertically centered on the screen.

    Good luck.

    0
  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Hello there,

    Your solution looks very good, I have a few suggestions.

    The container is not centered vertically, to place the container vertically center, just add the following style to your .container class

    .container {
   //add this property to your existing styles
   min-height: 100vh;
}

    Now you can remove margin-top property from .box class

    .box {
   //remove this style
   margin-top: 50px;
}

    I checeked the repo and I think its good to place your assets in separate folders, like images goes to 'images' folder and css files to you a 'css' folder.

    It is not a good practice to add inline styles to an HTML element (you added inline styles to a div with class box)

    Good luck with your future tasks

    Good day

    0
  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Congrats for completing the task.

    I have two points to make this layout better.

    I am using a wide monitor and the background image looks broken. You can make it better by adding following styles to your body tag

    body {
   //your exisitng styles
    background-repeat: repeat-x;
}

    Once you add this styles, the background image will cover the entire screen.

    When I open your page, I can see a scroll bar in the layout, its because body tag has default margin. As a quick fix, you can add margin:0px to your body tag

    body {
   //Your existing styles
   margin:0px;
}

    Now you will see the layout without a scroll bar

    Its recommended to have a reset.css file in the project to reset all the default styles. You can find sample reset file at https://meyerweb.com/eric/tools/css/reset/

    Good luck with your future tasks

    0
  • Joyal James•265
    @silkcoderPosted about 1 year ago

    Hello there,

    Congrats for completing your task, It looks good, however I have a few suggestions that can make your layout better

    First thing you have to keep in mind is that your layout get viewed in different sized devices. so is not a wise decision to set width for body tag. so you can remove width property from body tag

    body {
   //remove this
    width: 1440px
}

    Next, you can remove margin property you give to container div (we have a better method to center the container :) )

    .container {
   //remove following line
   margin: 200px 250px 150px 580px;
}

    Now the container get aliged to top left, that is not good :) we have to place the container centered in all sized devices. so apply the following propertiese to the body tag in addition to the existing styles

    body {
    //your styles here
    display: flex;
    justify-content: center;
    align-items: center;
    min-height: 100vh;
}

    Now the container get aligned to centered horzontally and vertically. Good job

    you can read more about flex box propertise at https://www.geeksforgeeks.org/introduction-to-css-flexbox/

    Now if you notice, you may see a scroll bar in the window, its because there is a margin property get added to body tag by default. so a recommended method to fix this is to use a reset css file

    [this is a sample reset css file] (https://meyerweb.com/eric/tools/css/reset/)

    you can download and load this css before loading your styles.css, this file reset all default styles get added to the HTML document

    As a quick fix (without using a reset file) you can add 'margin:0px' to your body tag to remove the scroll bar

    body {
   //all your styles in body tag goes here
   margin:0px;
}

    Now your layout look better.

    I noticed that on your repo, all files (images, css) are scattered, Its good to keep them on separate folders, so that all your images will stay on images folder and your styles to a styles folder

    Good luck with y our future tasks

    Good day

    0
View more comments