@pikapikamart
Posted
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
idto target and style an element since it is a bad practice due to css specificity. Instead, just useclassto 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
divas 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 afieldsetand a screen-reader onlylegendtag which it's text-content should describe what is the purpose of that section. You will need to use 2 radio buttons and 2labeltag, connected to their owninputtag. 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=""andaria-hidden="true"to theimgtag or onlyaria-hidden="true"if you are usingsvginstead ofimgtag. - For this one, it would be great to have a screen-reader only
h1inside of the supposedmaintag 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
formtag to wrap that layout since data is being used/controlled in there. - The search-icon
svgis decorative, usearia-hidden="true". - Your
inputright now currently lacks associatedlabelto it or anaria-labelto which will define the purpose of theinputelement. Always include it so that user will know what they need to give on eachinput. - For the filter-bar, currently when I tab on it, there is no visual indicator. Maybe checking or adding on the
:focus-visiblestate of theselecttag. - For each of the country flag
imgtag, you can just use only the country's name for thealtand 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
ptag or heading tag and not using likediv, spanto wrap the text.
VISITING A COUNTRY (on the site)
aria-hidden="true"for thego backicon.- Country flag could again use only the country's name as
alt. - The country should be using an
h1since it is the main content of the page. Changing theh2toh1would be really great. - For those 8 items, you should have only used a single
ultag on sine those are all related items/content. Useuland usedisplay: gridso that you could placed each item properly like in the design. - For the border-countries, you can as well use
ulon those since those are "list" of border countries.
Aside from those, great job again on this one.
Marked as helpful
@Sanchez9aa
Posted
@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!
@pikapikamart
Posted
@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: 21of 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 usemin-widthon those cards so that they will be wrapped onto another row if needed. - Thanks for the compliment about the theme-selector toggle:>
- Using a
formwill make the markup more clearer especially when screen-reader user interacts withinputfields asformadds extra information. Also you typically useformwhen theinputlike submits data onto whatever service. placeholderis not enough. Using alabel, a user would know what theinputneeds since thelabeltext 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-visiblemakes navigation on the site easier. For example here at frontendmentor, if you usetabkey 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 theflagword^^
Great that you find tips on this one. Goodluck on other challenges!
