Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added js #18

wants to merge 1 commit into from

Conversation

OrtalNisim
Copy link

No description provided.

Copy link
Member

@Tamir198 Tamir198 left a 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">
Copy link
Member

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');
Copy link
Member

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");

Copy link
Member

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())
Copy link
Member

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;
Copy link
Member

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">
Copy link
Member

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())
Copy link
Member

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 = "";
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants