-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
final countries pages without bonus #19
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general tips :
1 Do not use innerHTML
2 If you have the same code that repeats itself again and again extract it into a service that will expose methods.
3 save css hardcoded colors into variables
@@ -212,14 +211,32 @@ | |||
.countries-grid .country-info { | |||
position: relative; | |||
padding: 30px 25px; | |||
background-color: white; | |||
box-shadow: 0 2px 15px rgba(0, 0, 0, 0.08); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will want to save your colors into a css variables
const getDetailsOfCountry = () => { | ||
document.addEventListener("DOMContentLoaded", () => { | ||
const countryDetails = JSON.parse(localStorage.getItem("countryDetails")); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concepts should be extracted into a service so you wont repeat your logic again and again
details.js
Outdated
if (countryDetails) { | ||
console.log("Country Details on details.html:", countryDetails); | ||
document.querySelector(".main-country-flag").innerHTML = `<img class="country-flag" src="${countryDetails.flag}" alt="${countryDetails.name} flag" />`; | ||
document.querySelector(".country-name").innerHTML = `${countryDetails.name}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use innerHTML
it is a dangerous method.
|
||
|
||
const darkMode = () => { | ||
const lightBody = document.querySelector('.body'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If dark mode is a name of a function, you should give it more meaningful name.
When I read darkMode
it does not looks like a function name to me. more like a variable
index.js
Outdated
if (chosenRegion == 'america') { | ||
filteredCountries = filteredCountries.filter(country => country.region === 'Americas'); | ||
} | ||
if (chosenRegion == 'asia') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code repeat itself, you can do something like this :
const regionMap: Record<string, string> = {
america: 'Americas',
asia: 'Asia'
};
if (chosenRegion in regionMap) {
filteredCountries = filteredCountries.filter(country => country.region === regionMap[chosenRegion]);
}
In this way you will not repeat yourself again and again
No description provided.