Skip to content
OverviewSolutions
8
Comments
8
Kehinde Hussein, Fasunle
@Fasunle

All comments

  • Kehinde Hussein, Fasunle•190
    @FasunlePosted almost 2 years ago

    Hi @Deem,

    It is my pleasure to review your project. This is good for a starter and you can always become better. I will give a few tips that will help you do a better job.

    • Use can use either grid or flex. However, using flex is more flexible for this simple case. Using float or table is not recommended as this is very painful to scale in real life. That is not to say that they do not have their uses as well.

    • You can use picture tag to manage the two images without worrying about anything. Check this page to learn how to use it: https://www.w3schools.com/htmL/html_images_picture.asp

    • Read about semantic html. This will enable you to know which tag to use under different contexts and also, how they should be arranged/organized. Check: https://www.w3schools.com/htmL/html_images_picture.asp

    • On mobile screen size, ensure that your content does not take too much height. This prevents the image from getting too small (vertically). You can correct it by setting max-height for the content.

    If you find this useful, kindly mark it as helpful.

    Happy coding!!!

    0
  • Kehinde Hussein, Fasunle•190
    @FasunlePosted almost 2 years ago

    Hi @John,

    It is my pleasure to review your work. It is good how you combined everything. Since you want to make it even better, here are a few comments to make it even better:

    • The loading component was hardcoded. You could have used an image, icon or just svg image if you care about sizing and resolution. reference: https://github.com/jbidz/rest-countries/blob/main/src/components/Loading.jsx
    • The continent dropdown should have a default of Continent. Currently, the first child of the select element is used as default (Africa). Use:
    <option>Continent</option>

    reference: https://github.com/jbidz/rest-countries/blob/main/src/components/SelectByRegion.jsx

    • Each Card displays a raw population e.g 123342321. This is not very readable for big numbers. Use humanize to format the population in the card component. Here is a link to the npm package: https://www.npmjs.com/package/humanize-number.

    reference: https://github.com/jbidz/rest-countries/blob/main/src/components/Card.jsx

    I know this is a lot but it is rewarding to do.

    If you find this helpful, kindly mark it as helpful.

    Best Regards.

    Marked as helpful
    0
  • Kehinde Hussein, Fasunle•190
    @FasunlePosted almost 2 years ago

    Hey @Devalito67,

    It is my pleasure to review this project. You did an amazing job. Since you want a better job, here are a few tips that would make you result even better:

    • I find out that you are styling html elements directly e.g:
    
h3 {
  font-weight: 500;
  font-size: 12px;
  letter-spacing: 5px;
}

    This is not considered a good practice and it is pretty painful to understand the styles when you revisit your code in future. Instead,

    • reset the default styles: https://piccalil.li/blog/a-modern-css-reset/ to prevent inconsistencies across browsers.
    • use class instead of html elements e.g . heading instead of h2
    • BONUS: I would also suggest that you look into the BEM Methodology. This will make your work clean and fast.

    If you find this useful, kindly mark it as useful.

    Happy coding

    Marked as helpful
    1
  • Kehinde Hussein, Fasunle•190
    @FasunlePosted almost 2 years ago

    Hey @Boddaa,

    This is a nice attempt. I would like to make a few suggestions.

    Add image description i.e alt="Esmael's picture" so that it will be more accessible.

    You should be consistent with either mobile-first approach or desktop-first approach. This will ensure that you stick with either @media(min-width: ) or @media(max-width: )

    Additional: try BEM methodology in naming css classes. You will love it!

    1
  • dyl25•10
    @dyl25Submitted about 2 years ago

    • Is the CSS reset a thing that we still use in modern CSS ? Are there other better way to do it?

    • Did I use the correct way to center verticaly and horizontaly the main div (see .container class)?

    • Line height correct for the text below the QR code?

    QR code component

    1
    Kehinde Hussein, Fasunle•190
    @FasunlePosted about 2 years ago

    Hello dyl25,

    This is a really good solution. I would like to suggest that you use .container class on the main element instead of creating another element (div).

    Also, wrap img in a container like 'image-container' class. This is required for uniform rendering on some versions of internet explorer. It is also considered a best practice.

    img element must have attribute alt="description of the image". This is displayed if the image could not be loaded. However, if the image intended to be used just for design, empty string should be used instead. For example, <img src="my-image.png" alt="my profile picture"> or <img src="background-image.png" alt="">. This is also good for accessibility.

    I hope you find it useful.

    Marked as helpful
    0
  • Kehinde Hussein, Fasunle•190
    @FasunlePosted about 2 years ago

    Hey Olayemi,

    This is cool.

    These are my suggestions

    1 You could use the picture html element and declare the breakpoint in source element for desktop and default (i.e mobile) in img element. This would do the same job without using css and it will be clear as well neat.

    2 The product image should be the same size and product information section (i.e aside). You could use flex on parent container (i.e main element).

    3 Styling html elements (main, body etc) directly is not recommended and not good practice. Instead, make it a class and style it. This will save you a lot of time when you need to extend the project.

    I hope you find this helpful.

    0
  • Filip•110
    @fica25Submitted about 2 years ago

    Hi. I finished testimonials-grid-section with display grid of course. On design for mobile phone i used display flex. I found some stuff a little hard but I managed them at the end. I didnt have design file ,and I have smaller monitor than 1440px so I hope it will look good.

    testimonials grid section main

    3
    Kehinde Hussein, Fasunle•190
    @FasunlePosted about 2 years ago

    Hi Filip,

    You have done a great job. You just need to make some modifications for accessibility reasons. Make sure that all img tags have alt attribute with strings to summarize what the image is.

    For example, <img src="images/image-kira.jpg" class="profile-img" alt="App Logo" />

    Also, change all the backticks to single or double quotes.

    I hope you find this helpful.

    0
  • Assem•20
    @assem-frontdevSubmitted over 2 years ago
    • I struggle a bit when trying to centre the box both horizontally and vertically using flexbox and my question is: are there any other options to centre the element without giving the container 100vh?
    • I would appreciate your feedback on my work );

    QR code component solution using CSS Flexbox

    2
    Kehinde Hussein, Fasunle•190
    @FasunlePosted over 2 years ago

    HTML Landmarking

    div tag with class container should be the direct element of body tag i.e <div class="container"></div>

    use landmarking tags such as nav, header, main, footer to help with accessibility.

    don't use section tag directly inside body tag

    styles

    I suggest you don't give the container a fixed height of 100vh

    Just use display:flex; and center it by specifying justify-content: center; and align-items:center; on the container.

    You can add padding to the container. This will prevent your bar from using full 100% width on mobile.

    on you card component, you could set max-width: max-content; to prevent getting too big on larger screen.

    You could check my version of this and leave a comment.

    I hope you find this useful 😊

    0