Skip to content
OverviewSolutions
4
Comments
3
Lukasz
@lukaasz555

All comments

  • Dylan•160
    @dmanfrediSubmitted almost 2 years ago

    Easier than expected. Implementing the theme switcher was surprisingly painless. Not sure how accessible this is though. I'll see what the report says and go from there.

    REST Countries API w/ React

    1
    Lukasz•160
    @lukaasz555Posted almost 2 years ago

    Well done Dylan!

    I like the way you've made dark mode and how you used flexbox. I think you can improve some UX details, like:

    • dropped down menu, I think there would be a better idea to use <a> or <button> instead of <div> (clickable items) and so more, it's a little bit confusing that there is a cursor pointer but when I click the dropdown is closing. User need to click precisely at the word, All, Americas etc.

    • handle state, when there is no country provided in input, e.g. I typed Englang and I can't see nothing there:D

    • I think you could use more semantic marks instead of divs, e.g. you could use button inside filter container, 'cuz it's a clickable html element or you could replace div.countries-container by <main>;

    • another good feature is make a clickable header, but it's just idea:)

    Marked as helpful
    1
  • Lukasz•160
    @lukaasz555Posted almost 2 years ago

    Good job Himanshu, I like that you used semantic HTML (header, nav, footer). Clickable header is also good feature, but there is a still some features that you can improve, e.g.

    • you haven't done a view for mobile (good practice is to work mobile-first), I mean that your hamburger is not showing when I am resizing window;

    • LEARN MORE shoud be a clickable element (redirection isn't important at the moment) so you should use <a> or <button> instead of <h2>;

    • when I am resizing divs .five and .six I am loosing text;

    • ul.about & ul.media: it's similar to learn more. You should use <a> or <button> instead of <li>, of course, you can wrap e.g. <a> inside <li>;

    • the color of footer is wrong:P

    Keep coding:-)

    Marked as helpful
    0
  • Lukasz•160
    @lukaasz555Posted almost 2 years ago

    Hello, I think the good way to solve your problem is:

    • create [allCountries, setAllCountries];

    • create [filtered, setFiltered];

    In first useEffect, when you are fetching data from API, set respond as allCountries AND instead of fetching data once again and making another API call, you can interate over allCountries. You just need to render data from FILTERED instead of allCountries/countries.

    I've cloned your repo and it works, this is what I've changed in App.js file:

    const [allCountries, setAllCountries] = useState([]);

    const [filtered, setFiltered] = useState([]);

    Inside useEffect I just changed:

    setCountries(countries) to setAllCountries(countries) - ez;

    I've also added 2nd useEffect:

    useEffect(() => { setFiltered(allCountries); }, []);

    I've also changed all searchCountries, as you can see:

    const searchCountries = (e) => { if (allCountries.length > 0) { const inputValue = e.target.value.toLowerCase(); const filteredItems = allCountries.filter((item) => item.name.toLowerCase().includes(inputValue) ); setFiltered(filteredItems); } };

    And it's obvious, that now you have to add onChange listener to your input (and remove useRef): onChange={(e) => searchCountries(e)}

    Last, you have to render <Country {...props} /> component based at FILTERED, e.g.: filtered.map((country) => { return(<Country/>) }

    Marked as helpful
    1