Skip to content
OverviewSolutions
14
Comments
20
Joyal James
@silkcoder

All comments

  • Joyal James•265
    @silkcoder

    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
  • Joyal James•265
    @silkcoder

    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

  • Joyal James•265
    @silkcoder

    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.

  • Joyal James•265
    @silkcoder

    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

  • Joyal James•265
    @silkcoder

    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

  • Joyal James•265
    @silkcoder

    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

  • Joyal James•265
    @silkcoder

    Your solution looks good

    I have a suggestion about the max-width property you given to body tag.

    If your solution opened in a screen with width greater than 1440px, the content not get centered. So you can remove the max-width property of body tag

    body {
   //remove
   max-width: 1440px;
}

    Once you remove this property, your content remains on cener for all sized devices.

    Another point

    To center the container its wise to use Flexbox properties, read more about it on the page https://www.geeksforgeeks.org/introduction-to-css-flexbox/

    Marked as helpful
  • Joyal James•265
    @silkcoder

    Congrats for completing the task

    I noticed a few things

    • It would be great if you can keep the first item opened on page load
    • When you open an item, it would be great if it automatically closes already opened item
    • Your link to codebase is broken (GitHub repo link)

    Good luck with your future challenges. Have a good day

  • Joyal James•265
    @silkcoder

    Congrats for completing the task, it looks good.

    However, I have a suggestion to place the main tag content in the center of the screen horizontally and vertically.

    In your style.css file, add the following styles to body tag

    body {
    //add following styles to your existing styles
    display: flex;
    justify-content: center;
    align-items: center;
}

    remove 'margin-left: 35%;' and 'margin-top: 13.5%;' from your main tag styles

    main {
   //remove the following styles
   margin-left: 35%;
   margin-top: 13.5%;
}

    Now the main content get centered in all sized devices.

    You can read more about flex box styles in the following page Introduction to CSS Flexbox

    Good luck with your future challenges. Have a good day.

    Marked as helpful
  • Joyal James•265
    @silkcoder

    Congrats, good job

  • Joyal James•265
    @silkcoder

    Good work.

  • Joyal James•265
    @silkcoder

    Congrats for completing the task.

    I am adding a few points to improve your layout

    1. Mobile view need a different image.
    2. If you change mobile view orientation (rotate) then the layout breaks.
    3. Elements need position adjustments
    4. Text part need better width control on desktop view

    I also completed the same task today, have a look and let me know your comments, thanks

    All the best

    Marked as helpful
  • Joyal James•265
    @silkcoder

    Good work. but not sure why you have empty header and nav sections at the top.

  • Joyal James•265
    @silkcoder

    Nice work.

  • Joyal James•265
    @silkcoder

    Good work but the image on the right side is not visible. Please fix it.

  • Joyal James•265
    @silkcoder

    Good job. I think you forgot to add body background color. It will look better if you add a mouseover effect to the buttons on bottom.

  • Joyal James•265
    @silkcoder

    Good job, but when I previewed the site, I see the images on top are overlapping with the titles also the height is little different from the original design.

  • Joyal James•265
    @silkcoder

    Looks good, but I think it would be great if you can add the body background color too.