REST Countries API with color theme switcher With React

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. Desktop layout looks a bit different from the design. The header background-color should be white and the theme toggle look is different as well. Cards are much smaller and maybe reduce padding on the overall layout. The site is responsive but at size 490px, you can see the contents of the country cards are not overflowing its container. Mobile state looks fine though, just needed for the filter to not occupy full width.

  Some other suggestion on the site would be: -Avoid using height: 100% or height: 100vh on a large container like the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-height: 100vh so that the element will expand if it needs to.

  • Avoid using id to target and style an element since it is a bad practice due to css specificity. Instead, just use class to target element.
  • For this one, a more suitable markup would be something like:

  <header />
  <main />
  

  This way, all content of your page will be inside their own respective landmark element.

  • At the moment, your theme toggle works but not accessible. Remember that when creating interactive components, always use interactive elements. By using only a div as the toggle, you are excluding lots of users to try the functionality since it is only limited to mouse clicks, you can't interact with it by using keyboards or other peripherals. So for the theme toggle, a proper markup on that one would be to use a fieldset and a screen-reader only legend tag which it's text-content should describe what is the purpose of that section. You will need to use 2 radio buttons and 2 label tag, connected to their own input tag. Have a look at my solution on this one and see the markup of it. If you have any queries about it, just let me know okay^^.
  • Also, that moon-icon is only decorative so better hiding it. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if you are using svg instead of img tag.
  • For this one, it would be great to have a screen-reader only h1 inside of the supposed main tag so that it will describe what is the main-content is all about. You can see that as well in my solution.
  • For the search-bar, you could use form tag to wrap that layout since data is being used/controlled in there.
  • The search-icon svg is decorative, use aria-hidden="true".
  • Your input right now currently lacks associated label to it or an aria-label to which will define the purpose of the input element. Always include it so that user will know what they need to give on each input.
  • For the filter-bar, currently when I tab on it, there is no visual indicator. Maybe checking or adding on the :focus-visible state of the select tag.
  • For each of the country flag img tag, you can just use only the country's name for the alt and not use flag? I assume bandera is flag in english :>>
  • 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.

  VISITING A COUNTRY (on the site)

  • aria-hidden="true" for the go back icon.
  • Country flag could again use only the country's name as alt.
  • The country should be using an h1 since it is the main content of the page. Changing the h2 to h1 would be really great.
  • For those 8 items, you should have only used a single ul tag on sine those are all related items/content. Use ul and use display: grid so that you could placed each item properly like in the design.
  • For the border-countries, you can as well use ul on those since those are "list" of border countries.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Jesus Sanchez•40

  @Sanchez9aaPosted almost 3 years ago

  @pikapikamart

  Hi Raymart Pamplona, first of all, thank you for all this review and tips on how can I improve the app.

  I'm really excited about how you are dedicating your time to this. So I will do the same.

  I have fixed the colors of the headers and changed that border to box-shadow to make a more realistic design.

  Cards are smalled because it uses a max-width of 1100px, now is on 1300px and now is larger, no padding issue with this.

  About the overflowing in 490 - 570 px is now hidden but I wonder why it does not just wrap instead of overflowing, do you know why?

  Filter in mobile now is 50% width.

  The body now is min-height 100% you are absolutely right with this (and with all).

  Now root is a class and I will avoid using # in css3 according to good practices.

  You are right, have no sense of the header search filter markup that I was doing, now the main got the search filter component and is changed.

  I have never thought of that functionality that I was limiting with a div, huge info. I will replace div on a fieldset as the example, I will also change the dark mode animation, love example one.

  Did not know what aria-hidden does, thanks for the tip!

  Included that h1 to the doc and SEO, thanks!

  What would be the benefits to use a form instead of a div on the search bar?

  I did not use a label to make easier the style, is it good or integration practices? Is it not enough with a placeholder?

  Why is the need to include a focus visible is not needed in this design right? or is it for something else?

  I'm using the country's name, but I don't know with I used Spanish words like "Bandera de" instead of "flag of" ^^

  Visiting a country:

  Will try that display grid and that ul on the border.

  Again, thanks for all this time and tips, I have learned a lot with this!

  1

  

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  @Sanchez9aa Hey, glad that you found my feedback useful and sorry for this really late reply, I kind of rest up a bit doing reviews since I am coding my own project right now:>

  Just saw the updated site right now and it looks really great and just like the design! Also you should click the generate new screenshot so that it could update the old screenshot of the previews version of your solution.

  • The cards now looks much better since it is a bit bigger and the overall size of the site looks more occupied and no white-space that are left of.
  • For the overflowing of the cards content, it is because you are only using flex: 21 of the parent container and since on the 490px above, there are only 3 cards and they will continue being on the same row until the parent's container size is reduced. You can use min-width on those cards so that they will be wrapped onto another row if needed.
  • Thanks for the compliment about the theme-selector toggle:>
  • Using a form will make the markup more clearer especially when screen-reader user interacts with input fields as form adds extra information. Also you typically use form when the input like submits data onto whatever service.
  • placeholder is not enough. Using a label, a user would know what the input needs since the label text provides it. For example:

  <label>Enter first name</label>
  

  Also, placeholder I think is not translatable for other languages. Meaning if you use english as the placeholder text, when a user who is for example a japanese, the placeholder won't be translated to japanese, but the label will so it is much better to include it.

  • Using a focus-visible makes navigation on the site easier. For example here at frontendmentor, if you use tab key on your keyboard, when you focus on an element, it has this dash right, this is the indicator for the user where they are at. Imagine you have no indicator on your elements, use won't know where they are at when navigating your site using a keyboard or other peripherals.
  • For those, you can just use directly the country's name like alt="Philippines" and no need to include the flag or maybe you could, yeah, I think i'm wrong with this one :>> include the flag word^^

  Great that you find tips on this one. Goodluck on other challenges!

  0
• 

  hardy•3,660

  @hardy333Posted almost 3 years ago

  Hi, nice solution.

  You need to fix some issues on react side:

  • If user filters countries by region or by name from search input and then clicks country in order to see its' individual stats on separate page there are same react errors , you can replicate this error or just check this image out. Looks like you are incorrectly reaching to date and getting undefined but I am not 100% sure.

  • When use navigates from individual country page from main/home page you are making new request for all the data every time. This is not very optimal try to save response data on the first response and then use it over and over again.

  Marked as helpful

  0

  ---

  

  Jesus Sanchez•40

  @Sanchez9aaPosted almost 3 years ago

  @hardy333

  Will check that error, because on the report show me too that error that you are talking about.

  Definitely, I will save the response data and use it instead of making a new call, you are right, thanks for your time and tips!

  0