Skip to content
OverviewSolutions
23
Comments
7
Neil Khatri
@nkhatri7

All comments

  • Neil Khatri•620
    @nkhatri7Posted almost 3 years ago

    Hey well done on completing this challenge! The overall functionality seems to be pretty good. The only thing I'd suggest is adding hover states for the border countries buttons and the back button on the details page. Also if you want to make the flags look better on the cards, I'd suggest using object-fit: cover and making them all the same height and width so there's a consistent look. Other than that the design and code look great!

    Marked as helpful
    1
  • Benja.min•740
    @BenjaDotMinSubmitted almost 3 years ago

    Hello all! I decided to up the stakes and try my hand at a Junior level challenge.

    Here I learned how to:

    • fetch data from a json file
    • use the data to populate the DOM content (vanilla js, no frameworks)
    • swap the content on click, driven by the available data

    As always, any time spent is very much appreciated!

    Learning JSON and Fetch API (vanilla js)

    1
    Neil Khatri•620
    @nkhatri7Posted almost 3 years ago

    This is really well done! The javascript code is really efficient and just done well overall. The design is also really good, the only thing I'd call out is when the screen width is between around 700px to 1000px, the period filters are too spread out. I think it would be better if you did justify-content: center and then spaced them out a bit with margin (but still keep them rather centralised). Other than that, I can't really fault your code and design, keep up the good work!

    Marked as helpful
    2
  • Neil Khatri•620
    @nkhatri7Posted almost 3 years ago

    Hey Harsha,

    Just a couple of things that I think will be helpful:

    1. Try to use semantic HTML, so this would be using tags such as <main> and <footer> in your code. It's better for SEO (search engine optimisation) so it's a really good thing to have in your code. For this challenge, it would be good to wrap the card within <main> tags and the attribution within <footer> tags.
    2. Watch out for the indentation in your HTML file, it looks like your tags aren't indented properly. This can make it hard for yourself and others to understand your code.
    3. You should always have a <h1> tag, this is important for SEO once again and indicates what the most important text on a page is.
    4. To center your card, you can use absolute positioning in CSS: 
      position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
    5. Try to avoid using px as your unit in CSS and use more relative units such as rem.

    Hope this helps :)

    Marked as helpful
    0
  • Neil Khatri•620
    @nkhatri7Posted almost 3 years ago

    Hey Jaime,

    First of all the design looks great! One thing you should always try to remember is to use semantic HTML so in your case, it would be good to wrap your main content (the card) with <main> tags. Furthermore, you should always have a <h1> element, so instead of using <h3>, you should use <h1> as it's the main piece of text or heading for that page.

    Another thing is that for links that don't have any text (e.g. social media icons or images), you want to include an aria-label attribute in the <a> tag for accessibility purposes as it would help screen readers.

    Other than that, you're doing well so keep coding!

    0
  • Neil Khatri•620
    @nkhatri7Posted about 3 years ago

    Hey Nitish,

    Just a couple of issues I picked up with the UI and functionality:

    • There's no hover state for the explore button on the homepage
    • I noticed that with the destination headings, there is a weird font style with the line inside the letter, to fix this just simply write font-weight: lighter for the styling of those elements
    • The stats for the destinations don't show up when I view the destinations
    • When you first visit the crew page none of the option circles are active initially
    • On the technology page, with the buttons, when you hover over a button the background changes colour but the text does not which means that you can't see the text when you hover over a button. You can only see the text when you hover over the text as well.
    Marked as helpful
    0
  • Neil Khatri•620
    @nkhatri7Posted about 3 years ago

    Hey Igor,

    First of all, the design and code look great! So one way of adding more responsiveness is using vw and vh. These are relative units that respond to the dimensions of the viewport, so vw is 1% of the viewport width and vh is 1% of the viewport height. Now I wouldn't recommend using these relative units on their own, it's probably better to use them along with a base size in a calc() function (e.g. calc(10rem + 10vw)).

    P.S. I noticed in your HTML file you used the <head> tag inside your main, I'm assuming you wanted to use <header> instead?

    Hope this helps :)

    Marked as helpful
    1
  • Neil Khatri•620
    @nkhatri7Posted about 3 years ago

    Hey David,

    The layout and your code look great! The only thing I'd suggest is using Semantic HTML. So for example instead of writing <div class="main-content"> it would be better to use <main>. This blog post by Vincent Bidaux has a good explanation of what HTML5 semantic elements are and why they are beneficial.

    But other than that, great job!

    Marked as helpful
    1