-
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
added js #18
base: main
Are you sure you want to change the base?
added js #18
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.
Generally do not use innerHTML
Remove all of your comments - the code should explain itself
If you have a piece of code that you want to reuse extract it to a function
Extract hardcoded strings into a constants
@@ -38,7 +38,7 @@ | |||
</button> | |||
<!-- Header --> | |||
<header class="header"> | |||
<div class="container flex flex-jc-sb flex-ai-c"> | |||
<div id="container" class="container flex flex-jc-sb flex-ai-c"> | |||
<div class="logo"> |
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 already have class of container, why do you need also id ?
|
||
const changeBackgroundColor = () => { | ||
// Toggle the 'Dark' class on the <body> | ||
document.body.classList.toggle('Dark'); |
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.
In here please use styling via css classes and not js (this way you don`t need all of your logic)
// Extract country name | ||
const urlParams = new URLSearchParams(window.location.search); | ||
const countryName = urlParams.get("country"); | ||
|
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 dont need to add this comment, you already have a variable names countryName
} | ||
|
||
fetch("./CountriesData.json") | ||
.then((response) => response.json()) |
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.
Hardcoded strigns should be saved into constants variables
|
||
if (!country) { | ||
countryDetailsSection.innerHTML = `<p>Country details not found.</p>`; | ||
return; |
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.
Please do not use innerHTML
it is a dangerous method
} | ||
|
||
countryDetailsSection.innerHTML = ` | ||
<div class="country-details"> |
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 here
|
||
// Fetch data from CountriesData.json | ||
fetch("./CountriesData.json") | ||
.then((response) => response.json()) |
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.
Why not extract his into a function and call if getCountriesDate()
And call it instead of putting a comment
|
||
// Function to render countries | ||
const renderCountries = (data) => { | ||
countriesGrid.innerHTML = ""; |
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.
The name of the function should indicate what it is doing
No description provided.