@pikapikamart
Posted
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 thenav
tag on this one so that it will be contained properly. But to be honest, I wouldn't usenav
anda
tag for thewhere 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 noa
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
withlegend
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 usearia-hidden="true"
on it. - Change the
header
inside themain
tag to just use adiv
sinceheader
inside themain
treats it as a normal tag. You can instead useform
to wrap the search bar since data is being sent and manipulated in there. - The search-icon is not a great
label
for theinput
, you should have used that asbackground-image
and use a screen-reader onlylabel
for theinput
, 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 useselect
tag so that it will be more accessible. Another approach would be to userole="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 thea
tag that wraps theimg
tag, usedisplay: block
and create a custom indicator on thea
tag's:focus-visible
state. Useoutline
andoutline-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 ana
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 likediv, 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 linka
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 usearia-hidden="true"
on thesvg
, - 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.
Marked as helpful
@dulranga
Posted
@pikapikamart First of all, Thanks a lot for this feedback. I really appreciate it. I'm still studying about the accessibility and the layouts so yeah there are more to study.
I will fix all of your recommendations. and again thank you for the feedback. ❤️