Skip to content
OverviewSolutions
5
Comments
3
John
@JohnBroers

All comments

  • John•250
    @JohnBroers

    Really nice job on your solution. Looking spot on with the design. Your sass and javascript is looking well structured. Nice job on using mixins for breakpoints and using css variables.

    A couple things that i noticed in your javascript:

    1. I don't think you need to declare lat and lng variable in the global scope. You only set it once in the function, and pass is on to other functions as arguments. No need to declare it global i think.
    2. I see one 'var' variable which you might wanna change into a const.

    Another thing i noticed here in the accessibility report is that the submit button is missing a text label for screen readers. You might wanna add a screenreader only text element or add an aria-label to fix that issue.

    I agree with you that it was a fun challenge to work on, had a great time working on it as well.

    Marked as helpful
  • John•250
    @JohnBroers

    Nice work Folarin! Nice addition of the hover animation on the country blocks. A couple remarks and improvements from my side:

    1. The country blocks in the overview feel a little bit messy because of the different heights and sizes:
    • I would suggest setting a suitable fixed height on the flag images.
    • Make the flex items 'align-self: stretch' so all items in a row are equal height.

    1. I feel like the font-family is different then the design. Maybe that's design choice of your own. But be sure to set a font-family on the input and select elements because those ones are using the default browser font right now.

    2. Try to add thousand separators to the population numbers to make them more readable. (So 27,657,145 instead of 27657145)

    3. The country detail page keeps showing a loading spinner at the bottom. Even though i feel like everything has been loaded.

    4. I noticed a lot of console.logs when i used the search function. You might wanna remove that when you clean up your code.

    Your javascript code looks really well structured, nice job!

    Marked as helpful
  • John•250
    @JohnBroers

    Nice working Kevin, looking great compared to the design. Instead of using pseudo elements to place the background images you can also use multiple background images on the body element using css. And then play around with the background-size and background-position properties to place the images correctly.

    body {
    background-image: url('/bg-top.5a64f221.svg'), url('/bg-bottom.d04b6899.svg');
    background-repeat: no-repeat;
    background-position: top right, bottom left;
}

    And agreed with Iztk on the button issue. Try to add a transparant 1px border on the default state and give the border a color on hover, that should solve the layout jump.

    Marked as helpful