Shay Mashiah Country Assigment #21
Conversation
| @@ -0,0 +1,88 @@ | |||
| document.querySelector('.loader').style.display = 'none'; | |||
|
|
|||
There was a problem hiding this comment.
Change styles via css classes and not js
|
|
||
| if (countryName) { | ||
| fetch('./CountriesData.json') | ||
| .then(response => response.json()) |
There was a problem hiding this comment.
Hardcoded strings should be inside constants file
|
|
||
| const selectedCountryName = document.createElement('h1'); | ||
| selectedCountryName.innerHTML = selectedCountry.name; | ||
| selectedCountryName.classList.add('h1'); |
There was a problem hiding this comment.
Avoid using innerHTML, it is a dangerous method
|
|
||
| const countryPopulation = document.createElement('li'); | ||
| countryPopulation.innerHTML = `<strong>Population:</strong> ${selectedCountry.population}`; | ||
|
|
There was a problem hiding this comment.
You can extrac this into a function and call it inside a loop to prevent code duplication
| countryDataList.appendChild(countryCapital); | ||
| countryDataList.appendChild(countryDomain); | ||
| countryDataList.appendChild(countryCurrencies); | ||
| countryDataList.appendChild(countryLanguages); |
There was a problem hiding this comment.
This can be replaced with a loop :
const elements = [countryNativeName, countryPopulation, countryRegion, countrySubRegion];
elements.forEach(element => countryDataList.appendChild(element));And so on
| .then(response => response.json()) | ||
| .then(data => { | ||
| const container = document.querySelector('.countries-grid'); // המיכל שבו תוסיף את כל המדינות | ||
|
|
There was a problem hiding this comment.
Please do not use Hebrew comments, it will not support in every device, in addition the code should be self explanatory and understandable without comments
| const countries = document.getElementsByClassName('country'); | ||
| for (let i = 0; i < countries.length; i++) { | ||
| const countryRegion = countries[i].getElementsByClassName('country-brief')[0].getElementsByTagName('li')[1].innerText.split(':')[1].trim(); | ||
| if (region === 'All' || countryRegion === region) { |
There was a problem hiding this comment.
This line :
const countryRegion = countries[i].getElementsByClassName('country-brief')[0].getElementsByTagName('li')[1].innerText.split(':')[1].trim();
Is really long, you should split it into variables instead of really long line
No description provided.