REST Countries API with color theme switcher

Community feedback

• 

  Raymart Pamplona•16,040

  @pikapikamart

  Posted over 3 years ago

  Hey, really nice work on this one. The desktop layout looks great, the site is responsive and the mobile state looks really great as well. This is a nice solution on this one.

  Some other suggestions would be:

  • I think you forgot to remove some console.log on the site since I get some on the console:>
  • For this one, you should use header to wrap the nav tag on this one so that it will be contained properly. But to be honest, I wouldn't use nav and a tag for the where in the world since it will be hard to user to know where that link would take them since it is using a literal text. Because when you navigate on that, screen-reader will read that text and from that, it is ambiguous to where it will take the user. I would just use heading tag with no a tag for that.
  • Your theme toggle works fine but the markup on that one could be improved. A preferred markup for a theme toggle should be using radio buttons inside of a fieldset with legend tag as well. Have a look at this snippet of mine about a accessible theme toggle or you can view my solution on this same challenge. Let me know if you have queries about this one.
  • Also, that moon icon on the theme toggle is only decorative svg so use aria-hidden="true" on it.
  • Change the header inside the main tag to just use a div since header inside the main treats it as a normal tag. You can instead use form to wrap the search bar since data is being sent and manipulated in there.
  • The search-icon is not a great label for the input, you should have used that as background-image and use a screen-reader only label for the input, that would be much better.
  • Also, since you are implementing the search bar as "enter to search" kind of, it would be nice to have a aria-live element that will announce whether there is a country that matches the search item or if there are none.
  • For the filter bar, you shouldn't have just use button for that one since it doesn't really complement the dropdown that you created. For this, you can use select tag so that it will be more accessible. Another approach would be to use role="listbox" on the dropdown. On my solution, you can see the implementation of that one.
  • Also, when entering an empty value on the search bar, it fires a bunch of errors on the console. I found no way to bring back all the country cards after searching one. There should be something like a reset-button or some sort.
  • Right now, when navigation on each country card using tab key on keyboard, there is no visual-indicator to where I am at right now. For the a tag that wraps the img tag, use display: block and create a custom indicator on the a tag's :focus-visible state. Use outline and outline-offset on that one.
  • The country name should have just use a heading tag with no a tag since the image above them is already a link for the country to visit on the site. Also, wrapping a heading tag inside of an a tag is invalid markup. It should be the other way around.
  • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
  • For those 3 information for each country card, you can use ul on those since they are "list" of information about the country.

  VISITING A COUNTRY ON THE SITE:

  • go back should be a link a tag since it navigates a user in a another page. button are for controlling.
  • Also, the arrow-icon on the go back should be hidden so use aria-hidden="true" on the svg,
  • Those 8 items on the country are also "list" of information so it would be nice to use ul on those.
  • You are missing the border-countries on this one. Have a check for that.

  Aside from those, great job again on this one.

  11

  Marked as helpful