This is the first time I have used ChartJS in any project, so I had to learn the basics of it before I could get started. Overall it was a fun experience plus I will be looking to do projects of this type i.e Data Representation.
Please go through my solution and if there are any suggestions/tips then let me know. Thank you.
Hi 👋,
Good work on ChartJS usage 🚀.
I have a couple of suggestions/improvements:
HTML
[1. Any reason you load bootstrap.grid.css and a bunch of fonts that are not used anywhere?
[2. I think headings are misused. For inspiration, look at other solutions, e.g. https://www.frontendmentor.io/solutions/responsive-bar-chart-component-with-json-data-fetching-b2LJ5eTGIX.
[3. There is a typo on line: <div class="container" width="100px" ,height="100px"> you need to remove comma. Also, I do not think setting width and height is valid; you can specify it as style but not attribute.
JS
[4. You have a typo on this line events: ["mousmove", "click"],, it should be mousemove; otherwise hover effect does not work. Do you have any specific reason to limit events?
[5. To match the design, you can remove the horizontal line below the bar element:
x: { grid: { display: false, // Add this line to remove the bottom line drawBorder: false, }, }
[6. The y-axis configuration is set twice, which means only a second try will be preserved.
{ scales: { ... y: { // this will be lost grid: { display:false } }, y: { // this will stay display :false } } }
You can specify all options like this:
y: { display :false, grid: { display:false, } },
[7. To make sure border-radius is set everywhere, you can use the borderSkipped option:
options: { borderSkipped: false, ... }
Check API how it works https://www.chartjs.org/docs/latest/charts/bar.html#borderskipped
General
[8. To name your classes consistently, check BEM, and to structure it in a reusable way, take a look at CUBE CSS. Check these resources for additional information:
[9. Use CSS reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
I think the eye icon on the overlay image is not well centered, How can I solve this? Is ok the opacity of the eye icon?
any other suggestion will be well received.
thanks
Hi 👋,
Good work on this challenge 🚀.
I have a couple of suggestions/improvements:
HTML
[1. The anchor element cannot have another anchor element as a child.
CSS
[2. As .overlay has already correct position and size, one way to center child elements would be:
.overlay { display: flex; justify-content: center; align-items: center; }
The icon itself does not need any styling in this case except the colour.
[3. While the solution for centering above is different, it would be helpful to understand the cause of the previous issue.
The child anchor element's height is calculated using font, font-size and line-height, and then img is placed on the text baseline. You can try to change vertical-align on the image to see how it changes the position. The height is huge compared to the image, and the image is placed similar to how the text would be placed. You can, for example, fix that by setting font-size: 0 on the anchor element, but in your case, you just remove the child anchor element. Here is related material:
[4. When the viewport height is small, the card is only partially visible, and there is no scrollbar. You can change the body element height style to min-height. Also, a bit of padding applied to the main element will add space otherwise on smaller screens it is on the edge of the viewport.
General
[5. Use css reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
first time trying with typescript any suggestions ?
Hi 👋,
Nice job 🚀. I noticed a couple of things that can be improved.
HTML
[1. Use button tag instead of div tag for icon buttons, and do not forget to add aria-label for buttons with no text.
[2. Usually, the logo is a link to the home page with an alt property set to the app name.
[3. Check responsiveness on screen size e.g. 692px.
JS/React/Typescript
[4. Typescript looks good; when possible, create reusable types, e.g.:
interface Image { poster: string; thumbnail: string; }
And use it whenever this type is used.
[5. If you use hard-coded data, move it out of the component, nothing is happening with it while rendering. A better approach would be to imitate fetch using a slight delay with a loading indicator to mimic the real application.
[6. The first parameter of the array splice method is the index at which to start changing the array. Using id to remove the item from the cart as the first parameter of splice will not work correctly.
[7. You can skip <>...<> as the root element when there is only one child element.
[8. Use the header tag as a root element of the Header component instead of wrapping the Header component in App.
[9. In Cart component you check if cart is empty several times, it is a good candidate to have it in a variable e.g. const isPresent = data?.length > 0.
[10. useState<boolean>(false) you can skip type here if you expect the value to be only boolean as typescript will know.
[11. NavLinks will probably be reused in other places .e.g in the footer, so keep it outside the component and/or move it to its file.
General
[12. Use reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
[13. For commit messages, it will help others if you add a descriptive message and follow a specific format, so all is consistent https://www.conventionalcommits.org/en/v1.0.0/. You can even make a check before committing if you follow those rules by using a tool like commitlint - https://commitlint.js.org/#/
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Can i optimalize that ?
Hi 👋,
Nice job 🚀. I noticed a couple of things that can be improved.
HTML
[1. Do not use main tag as a wrapper of card, instead add parent div or section tag for your card <main><div class='panel'>...</div></main> as a child of main tag. main tag specifies the main content of a document https://www.w3schools.com/html/html5_semantic_elements.asp
[2. Rate-related elements with submit button can be recreated and wrapped with the form, which is a better way to handle this case. Instead of a button for each rate, you can use radio input as it matches the required logic choosing one from several. You will learn a lot by checking other solutions after your attempt, e.g. check the solution for the same challenge of @Elaine https://www.frontendmentor.io/solutions/responsive-interactive-rating-component-SeBo-aR4gB it is implemented using form and radio inputs; it has consistent names and implemented well.
CSS
[3. The main tag used as a card wrapper has a width: 25vw without restriction on huge screens; your card will be pretty wide. Instead, you can set the width to 100%, but restrict it not to be greater than 400px using max-width: 400px.
[4. It looks like you are using BEM for naming classes; if yes, I think it is not used correctly. For example .Card__starbox--img - img is not a modifier but an element, instead you can use something like card__sticker, same for .Card__Submitbox--submit. .Card__Rating---rate has three dashes, and .second-image is not part of the card - consistency will make your code easy to read and maintain.
JS
[5. You don't need to apply styles from javascript in this case. Define them in CSS and use the class name to reference those styles.
[6. For a map of a similar key/value in the getRate function, why not use Amount directly?
[7. While there is nothing wrong with string concatenation, you can also check template literals.
[8. Getting the answer value can be moved into submit click listener, as it is needed only on submit, not before.
General
[9. Use reset to have a good starting point, e.g. https://piccalil.li/blog/a-modern-css-reset/.
[10. There is only one commit which might be fine for a small project, but even on a small project, train yourself and commit often with a meaningful message when specific work is completed. For a consistent format of commit messages, you can use (https://www.conventionalcommits.org/en/v1.0.0/).
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
I used CSS grid for this task and came across a problem. When I made the grid layout for 375-px-wide screens I changed the number of columns to 1. For desktop devices I had 2 columns, and expected that setting grip-template-columns to 1 would change it. However, it did not work. All my sections are in 1 column as I intended them to be, but there is another column, which does not contain anything, and it creates extra redundant space on the right which I don't know how to remove. The question is, how do I remove that column?
Hi 👋,
Good work on this challenge 🚀.
CSS
[1. Regarding your question. You have width: 50% on body main .elements which sets elements width to 50%. The grid works as expected; it is not an extra column but one with elements width set to 50%. In Chrome, there is a grid badge/toggle under the Elements tab of DevTools for the elements with display: grid; by toggling it, you will see the outline and additional info about the grid.
[2. Use CSS reset to have a good starting point, for example:
[3. You will have issues if you customize styles for specific text lengths. You need to make a layout adaptable to any text length, as the length of the text is unpredictable. Start by removing fixed widths, allow elements to grow and restrict using (max|min)-(width|height) when needed; remove unnecessary margins.
HTML
[4. Do not use main tag as a wrapper of your panel, instead add div tag for your panel <main><div class='panel'>...</div></main> as a child of main tag.
General
[5. Take a look at a different approach where you first define default styles for narrow screens and add features for wider screens.
[6. To name your classes consistently, check BEM, and to structure it in a reusable way, take a look at CUBE CSS. It will allow to reuse styles or make them easily customizable without deep nesting. Imagine a situation where you need to inject another element before body main .elements:nth-of-type(2) button { this will require changes that are not straightforward, compared to if you had .panel__subscribe button, or even better to style .button so it can be reused. Check these resources for additional information:
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Is there any way to implement this design without using the placeholder attribute?
Hi 👋,
Excellent work on this challenge 🚀. It looks fantastic on different screen sizes.
[1. Regarding your question. You can use a label on top of the input, and when the input is focused, either hide or move the label to a different place. You can check the real example here https://mui.com/material-ui/react-text-field/#basic-textfield; inspect input to see all elements.
[2. While using the screen reader, the input error is read only once. Later, if you focus on input with an error, there is no indication that the input is invalid. Using aria-invalid and aria-errormessage or aria-describedby might help with it https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-errormessage.
[3. Submit event listener is added for each input; is it intentional?
[4. When input text is long, and the error icon is visible, the text overlaps with the icon. Increasing padding on the right side will help to fix it.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Any feedback is welcome.
Hi 👋,
Nice job 🚀. Noticed a couple of things that can be improved.
CSS
[1. The header is not showing all links in some cases:
[2. When the mobile modal is open, navigation links do not expand to show inner child elements.
[3. Element with .top class has min-height: 80vh, on bigger screens text in the hero section is not centred vertically. Also at some point, if you start decreasing height, the login button of the hero section will be partially invisible. I think removing min-height and adding bottom padding will do the trick.
[4.Start for Free like buttons have no pointer cursor and lack the animation you have on the Login button.
[5. main section.content .contentBox .content_right .imgBox.image-left is a bit long and redundant, you might benefit from using BEM https://getbem.com/naming/ e.g. info-section__image and .info-section__image--left.
[6. Spaces/sizes are a bit off; if you use a slider on the solution page, you can see the difference.
I hope this will be helpful.
Keep up the good work 🚀. Cheers!
All feedback is welcome
Hi ,
Great job, it looks good.
I have a couple of suggestions:
CSS
[1. All elements inside .whole have margin-left: 40px. To achieve the same but to have only one style, you can define padding on the .whole element. It will be easier in the future to change it instead of specifying it for each child element.
/* remove all margin-left: 40px on all child elements and add one padding to parent */ .whole { padding: 17px 40px; }
[2. The content inside the Add to Cart button is not aligned vertically; it looks like the fixed height is redundant.
.whole .fa { /* width: 50px; */ /* height: 100px; */ margin-right: 20px; }
[3. You have fixed width and height which works in this case, but you cannot depend on text length; in real applications, you never know the size of the length, so you need to make your layout adapt to whatever length it will have. You can check it by typing longer text for an item description. Allow elements to grow without fixed width/height, and when you want to restrict, use max-width/max-height or min-width/min-height;
[4. On smaller screens, e.g. 320px, the page doesn't look like in mock. You might need to use media queries, so it looks good on smaller screens.
And for arranging items, try using flexbox or grid as it is easier to use and has more features.
Using a mobile-first approach and media queries, you will change styles to:
.container { /* A B */ display: flex; flex-direction: column; /* you can set radius on parent element instead of each child element */ border-radius: 10px; overflow: hidden; } @media (min-width: 900px) { .container { /* A B */ /* display: flex; */ flex-direction: row; } }
[5. You can set display: block on the image to remove white space under the image. For details, check:
HTML
[6. Wrap your card container with the main tag instead of using the main tag as your card parent tag; this would be semantically correct.
Let me know if you have questions. I hope this will be helpful.
Keep up the good work . Cheers!
need your feedback for further improvements. solution
Hi 👋,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
HTML/CSS
[1. Country flag images look distorted on the main and the details page. On the main page, you can remove height so it can grow based on width. And the details page, instead of specifying height, you can remove the height attribute and limit max-width to a value of 400px, for example.
[2. The class names are a bit long; it will be hard to work with them in the future. As you are already using BEM (http://getbem.com/naming/), instead of
country-details__content__details__information
you can have
country-details__infocountry-details__content__details__information__container-1
country-details__info-maincountry-details__content__details__information__container-2 - country-details__info-secondary[3. As far as I understood, the border country elements on details pages should be a link to a country itself.
JS/REACT
[4. For HTML Validations error that can be seen in the report, I think all is related to illegal characters in href, which you can fix by wrapping URL params in encodeURI.
(https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURI,
https://stackoverflow.com/questions/332872/encode-url-in-javascript)
[5. For numbers to be user-friendly, format them so that instead of 35000322, you will have 35,000,322. (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat)
[6. When you search for a country, and the result is empty, it shows the last result. Should not it be an empty list?
[7. It is better to be consistent and use one format for variable names. For example, you have filterref, searchRef, and country_results. (https://javascript.info/variables#variable-naming)
[8. Using constant for API URL or helper method will allow to change it quickly, and it will not be hardcoded in multiple places.
const API_URL = "https://restcountries.com/v2/"; fetch(`${API_URL}/name/${param.countryname}?fullText=true`);
[9. The value of mode is passed to each component that needs it. There is a term for it prop drilling, which is okay in some cases, but as this value is shared across all application components, you can use Provider Pattern (https://www.patterns.dev/posts/provider-pattern/) instead, which is using Context (https://reactjs.org/docs/context.html). Using this pattern will remove unnecessary duplication and be more maintainable. styled-components has a helper component to do precisely that - https://styled-components.com/docs/advanced#theming.
[10. Regarding variable name mode - it is hard to understand the meaning without reading the code; instead, you can name it isDark | isLight | theme if it has 'dark', 'light' values.
[11. In MainContent, you are using ref for input. Why not directly use useState (https://www.w3schools.com/react/react_forms.asp)?
[12. For the select field, you will benefit from using libraries like https://react-select.com/home as it is flexible and follows best practices.
[13. You can use useMemo to avoid unnecessary processing/calculations and be more performant. It will only recalculate the value of filteredCountries on filter or country_results change. Example:
// https://reactjs.org/docs/hooks-reference.html#usememo
const filteredCountries = useMemo(() => {
if (filter) {
const filterValue = filter.toLowerCase();
/* You can also prepare lowercase region name on the cleaning stage, so it is done only once and not on each call of useMemo */
return country_results.filter((value) => value.region.toLowerCase() === filterValue);
}
return country_results;
}, [filter, country_results]);
...
<Grid className="grid">
{filteredCountries.length > 0 && filteredCountries.map((country, countryIndex) =>
<GridCard result={country} key={countryIndex} mode={props.mode} />
)}
</Grid>
[14. I noticed that data is fetched every time user navigates to country-api. What do you think of getting all data once and then reusing it?
GENERAL
[15. For some reason, when you reload the detail page, it shows a 404 error.
[16. You can use prettier, which can format your code automatically on save, so code formatting is consistent. For example, you will not have two different formats:
const CountryInDetails = ({ mode }) => {
const NavBar=(props)=>{
https://create-react-app.dev/docs/setting-up-your-editor/#formatting-code-automatically
[17. For commit messages, it will help others if you add a descriptive message and follow a specific format, so all is consistent (https://www.conventionalcommits.org/en/v1.0.0/). You can even make a check before committing if you follow those rules by using a tool like commitlint - https://commitlint.js.org/#/
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
What's your opinion ?
Hi 👋,
Great job, it looks really good.
I have a couple of suggestions/improvements, though:
HTML
[1. Wrap your content with the main tag. That way browser will know where to find primary content. Also, it will fix some accessibility issues. Here is a resource with examples related to semantic tags.
[2. Add alt attribute to img tag. It can be empty or should describe what is on the image. Screen reader users should have an idea of what is on the image based on the alt text. Here are some resources: mdn alt, great-alt-text.
[3. HTML and CSS files are not formatted properly. It will make your code more readable if you use an editor extension that can format on each save for you. For example, if you are using vscode try prettier, for different editor search for editor-name prettier.
CSS
[4. Consider more universal approach for box-sizing using this method.
[5. Take a look at this thread regarding different ways to name CSS colour variables, so it is easier to use while styling your app. slack discussion.
[6. You are using :nth-of-type(N) pseudo-class a lot, which looks not very efficient. Next time you want to insert another card in the middle, you will have to change the rest; instead, you can create .card--bg-pink or .bg-pink and apply where necessary. Another example would be to have modifier .card--with-quote-bg, which will add quote background. That way, you make your code easier to read; and easier to apply changes. Here is a resource on how to structure your CSS using [BEM methodology] (http://getbem.com/introduction/), also later, you might be interested in this scss and this tailwindcss.
[7. Where possible, try not to use fixed width and height so your elements can adapt to different screen sizes, limit using max-, min- version of width and height where required but allow to adapt. Also, it feels like not the best choice to use em, unless that was the intention to bond width to font size. One of the possible solutions using grid but without fixed width/height would be to remove all width/height except on containers and then have something like this:
/* For this to work first in HTML, move current card#5 to be card#3 so it is ordered the way they appear. */
.card {
height: 100%;
}
@media(min-width: 1200px) {
.container {
padding: 20px;
}
.grid-wrapper {
display: grid;
grid-template-columns: repeat(4, 1fr);
grid-gap: 20px;
}
.card:nth-of-type(1) {
grid-column: 1 / 3;
grid-row: 1;
}
.card:nth-of-type(2) {
grid-column: 3;
grid-row: 1;
}
.card:nth-of-type(3) {
grid-column: 4;
grid-row: 1 / 3;
}
.card:nth-of-type(4) {
grid-column: 1;
grid-row: 2;
}
.card:nth-of-type(5) {
grid-column: 2 / 4;
grid-row: 2;
}
}
There are other ways to achieve this, e.g. using grid-template-areas.
[8. It would look better if you move background-color to the body tag, so all area is filled with background.
[9. Avatar size is not equal as it has width: 12%, so it depends on the size of the container. In this case, you might benefit from using fixed sizes as you know it should have the same size on all cards.
.img-div { width: 45px; height: 45px; }
Let me know if you have questions. I hope this will be helpful.
Keep up the good work 🚀. Cheers!
Fun project. Feedback welcome.
Hello Carl,
Great job, it looks really good.
Couple ideas/suggestions/improvements for your implementation:
JS
[1. The if/else check of window.innerWidth looks redundant you may simplify by:
// original code
if (window.innerWidth > 1439) {
setIsMaxWidth(true);
} else if (window.innerWidth <= 1439) {
setIsMaxWidth(false);
}
// instead you can write this
setIsMaxWidth(window.innerWidth > 1439)
[2. From the code, it looks like menuHidden, isMaxWidth, tabs is only used inside specific components, so why have them outside then? My rule is to put things as close as possible where they are needed. Also, moving the tabs variable outside the component will not recreate it on each re-render; it is static data that does not depend on anything.
The same goes for desktopHeros and mobile mobileHeros. Put it outside of the components. Also, there is no need for those variables as the only thing that is changing is the number that you can get from the slide number.
HTML
[3. Where possible, use existing elements, e.g. instead of div to mimic buttons, use the button tag and restyle it as you need. That way, it will retain native browser behaviour. For lists, use ul/li; other elements that can be used are header, nav; take a look at semantic elements and why they are used html-semantics-cheat-sheet.
CSS
[4. Regarding responsiveness, it looks strange to see one small column on the desktop when the screen is 1400px in width. You are heavily using fixed width and heights maybe using img tags and max-, min- versions of width/height may help to make it adaptable to other screen sizes.
[5. While there is nothing wrong with using BEM and CSS modules together, it looks redundant to me. With CSS modules, there is no need to scope class names as they will be already unique. That is, the whole point css.button in one component is different than css.button in another component. Also, you may use some nesting like .about h2 {}. You will get rid of the css[''] style as you can easily find simple/short names for all scoped elements. Take a look at other examples css-modules-examples.
If you find yourself using a lot of conditions inside className, you may take a look at this library clsx, or classnames.
React
[6. As the project's design suggests, there should be other pages; having this in mind even for small projects will help later. For example, having a pages folder with the LandingPage component and moving all related components inside. Or building a general Layout component and using it inside LandingPage as the wrapper.
[7. In App component window.addEventListener("resize", will be called on every render, which means you are adding more event listeners on each re-render which I guess was not your intention. If you move that code into useEffect(..., []), this will be called only once on the mount, also in useEffect, you can remove the event listener on component unmount using the return method. For inspiration, you may check this library of hooks and particularly react-use which does the same thing as you want.
[8. In the TopBar component, instead of className conditions everywhere, you may set className on the parent like ...--desktop, and for all that need the same check, use that like ...--desktop .topbar__nav-items inside CSS.
[9. As HeroImage and PageContent share state and data (as each slider image has different text), you might combine them inside the wrapper component.
===
I hope this will be helpful.
Keep up the good work!
Hello community, that's my first FEM project in React and the second i ever made using this library. It has been a long journey, i'm satisfied with the result but i focused most of my attention and energy on the functionality part (it kept me very busy) of the project. I decided to use plain CSS, i tried to avoid repetition except for the theme switcher, i couldn't avoid WET code which means that maybe the whole functionality could have been implemented in a better way.
I thought it was nice having a load more button and a back-to-top button so i added them. I could disable the load more button at the end of the page but just for all countries, it would be nice making it work for each filter, i'm not there yet.
I've never used select element, it turned out to be very hard to style especially in Google Chrome it appears worse than Firefox, i tried my best! Dark mode looks ok on mobile but on desktop the arrow down disappears.
The API changed. That happened nearly at the end of the project, the new endpoint was unstable for over a week, at the moment is working great! However there's an important difference between the two. The old API had the border property in all countries, no matter if a country has no border country, the property was there and of course the value was 0, i structured my code so that it worked perfectly! The new API didn't include the property in all of those countries with no borders, and that meant i had to change the code structure to make it perfect, so you are going to see a blank page when you click some countries.
To conclude, altough there's room for improvement i'm quite happy i could work on an advanced challenge, i learned a lot and i want to learn more about React, patterns, architecture and styled components is the next goal.
Feel free to leave a feedback.
Hello Davide,
Great start with React 🚀.
I like the idea of the load-more button that you added 👍. Couple ideas/suggestions/improvements for your implementation:
Issues
[1. If you search for a country that is not yet visible, the list will be empty. It is happening because there is a slice of data before filtering. Changing the order of slice and filter should do the trick.
[2. When country borders is not defined single-country page raises an error. For starters, you can assign an empty array, so it is working.
const {
alpha2Code,
...
// if borders is undefined it will set it to empty array
borders = []
} = country;
Take a look at Destructuring search for "Default values" for an object.
[3. The load more button stays on the page, even when filtered data has less data than the visibleCards amount. A simple condition will fix that.
[4. If you reload the single-country page or navigate to it directly, there is an error in the console. That is caused because at this point country list is not loaded yet.
React
[5. Countries page is calculating countries variable every time it renders. The trigger of render might be something other than search/filter/visibleCard change. One of the ways to eliminate unnecessary calculation is to use useEffect({...setFilteredCountries(...filtered countries) }, [query, fitler, visibleCards]). And inside component return {filteredCountries.map((c) => <Card ... />)}.
[6. As far as I can see, the API does not support pagination, but it supports fetching specific country/countries. For the single-country page, there is no need to know about all countries. You might load specific country data directly and after it border countries; even better before that, checking context if all countries are already loaded and if yes, get it from there instead. What I am saying is that fetching all countries on the single-country page is redundant.
[7. Currently, a global context has data related to the theme, country list, query, and filter. If we check the usage of those, it would be: theme: whole application country list: countries page/maybe single country page query: only countries page filterData: only countries page
It looks like only theme/maybe countries should be accessible by the whole app, and the rest can be moved to a specific page. If you keep adding new pages to your app, those pages will probably not need to fetch allCountries, and they do not need to have query/filterData. The simplest solution would be to move query/filterData state to Countries page, and trigger fetching all countries from Countries page but save it in context as you have it right now. And for the single-country page to check if allCountries is already loaded, use that if not - fetch single country data.
[8. It would be nice to have a loading indicator while filtering/loading more and fetching single country data.
[9. You might reset visibleCards to the initial value every time you use a query or filter.
[10. On the Countries page, the same logic is executed for country filtering. You can combine those:
if (item.region === filterData) {
...same
}
else if (filterData === "All") {
...same
}
// instead you can do
if (filterData === 'All' || item.region === filterData) {
...same
}
}
Also, you can improve it by checking query value. There is no need to use a query if it is an empty string.
[11. main tag element and `back-to-top' are excellent candidates to move to separate component and reuse on all pages or move it to the app component. e.g.:
const Layout = ({children}) => (
<main className="main-content">{children}</main>
<BackToTop />
)
[12. const [search] = useState(["name"]); - there is no need to have it as a state you can have it as a const out of the component. e.g.: const searchFields = ['name'];
[13. There are both versions in code; I suggest sticking to the last one as you are not repeating yourself.
<i className= {darkMode ? "fa fa-search icon darkmode" : "fa fa-search icon"}></i>
<i className={`fa fa-search icon${darkMode ? "darkmode" : ""}}></i>
If you find yourself using a lot of conditions inside className, you may take a look at this libraries clsx, classnames
JS
[14. Instead of classList.add() and classList.remove() you can use classList.toggle("className", flag)
[15. Things like the number of items per page are a good candidate to move to a constant and out of component to be reused, e.g. const COUNTRIES_PER_PAGE=49.
[16. There is no need to call query.toLowerCase() each time you check if it is part of the country name. As query is not changing, set a variable before filtering and use it instead. It might be fast now, but there can be some heavy calculation involved in the called method in other applications.
HTML
[17. Currently, users need to guess that card is clickable unless the user hovers the image. If you make the whole card clickable, it might be better for the user experience.
[18. The usage of semantic elements like footer and article is misleading. I would make a footer child of body and place content inside, and the article will contain all country info.
[19. Instead of alt="Country flag route", you can change it to include country name so that the screen reader will read to which country user will be navigated.
[20. Change theme switch label when changing theme for better user experience Light Mode/Dark Mode.
[21. To write less instead of writing lists by hand:
<option value="All">Filter by Region</option>
<option value="Africa">Africa</option>
<option value="Americas">America</option>
...
// you can create an array of regions
const regions = ['All', 'Africa', 'Americas' ...];
// and later output
{regions.map((region) => (<option key={region} value={region}>{region}</option>)}
// cleaner code, easier to update, you can later use API to get all regions
CSS
[22. The issue with the select input arrow icon can be fixed by removing background-image: inherit; from .select-dark, and linear-gradient from:
select,
.select-dark {
/* replace this */
background-image: url('data:image/svg+xml;...'), linear-gradient(to bottom, #ffffff 0%,#fff 100%);
/* with this */
background-image: url('data:image/svg+xml;...');
}
That should do the trick. In general, it is hard to style select input. Design of select input is much more difficult most of the time, so you will be forced to use an external library. Here are examples of libraries that you can use instead: For react react-select For js select2
[23. You are already using the theme class on the body tag instead of having theme-related classes on each element. You can reuse parts of element styles that are shared and apply only differences based on body theme class.
.btn {
/*
all shared styles
and one for the light theme
*/
}
.dark .btn {
/*
specific to dark theme
*/
}
By doing that you can remove code like <div className= {darkMode ? "country-info-dark" : "country-info"}> as you will just have <div className="country-info"> and body theme class will trigger theme specific overwrites.
[24. There are minor differences when switching from light to dark:
.country-detail > p font-weight is changing to lighter for some reason.Resources
To format code and be consistent, you can use Prettier. Also, you can add configuration so every time you commit, it formats it CRA-Formatting Code Automatically
Take a look at another style for className naming format BEM
As I said only have a couple of suggestions 😬😅. I hope it is not overwhelming and will be helpful.
Keep up the good work! Cheers!