cr pull request #3
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Some things that you should fix generally :
Avoid Using innerHTML
Extract Repeating Concepts into a Service
| <!-- <script src="./js/common.js"></script> | ||
| <script src="./js/details.js"></script> --> | ||
| <script src="script.js"></script> | ||
| </body> |
There was a problem hiding this comment.
Delete code dont comment it out
| if (countryFetchedData) { | ||
| return; | ||
| } | ||
| try { |
There was a problem hiding this comment.
-
you can write this like
if (countryFetchedData) return -
This function should do 1 thing and that fetch the data I recommend writing your logic before calling the function
| } | ||
| try { | ||
| const response = await fetch('./CountriesData.json'); | ||
| if (!response.ok) { |
There was a problem hiding this comment.
Also you can save your strings inside constants file or variable and not use them as hardcoded values
| function getDropdownsRegions(data) { | ||
| const dropdownRegionFilter = document.querySelector('.dropdown-body'); | ||
| const uniqueRegionsSet = new Set(data.map(country => country.region)); | ||
| const uniqueRegions = [...uniqueRegionsSet] |
There was a problem hiding this comment.
In here you can destruct your object, it is generally a good practice:
const uniqueRegionsSet = new Set(data.map(({ region }) => region));
| analyzeMainContainer(filteredData); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
-
Never use
innetHTMl, its a dangerous method -
The name of the function does not reflect what the function is actually doing (adding event listener is not getting data). so please separate your code into helper functions
| const countryData = { name, flag, population, region, capital }; | ||
| localStorage.setItem('countryDetails', JSON.stringify(countryData)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Instead of using localStorage like this, create a service that expose method related to localStorage.
This will help you avoid repeat your code and seperate different concepts
| data.forEach(({ name, flag, population, region, capital }) => { | ||
| const countryCard = document.createElement('a'); | ||
| countryCard.href = './details.html'; | ||
| countryCard.addEventListener('click', function () { |
There was a problem hiding this comment.
Same thing here about constants
|
|
||
| countryCard.innerHTML = ` | ||
| <div class="country-flag"> | ||
| <img src="${flag}" alt="${name} Flag"> |
There was a problem hiding this comment.
Same comment in here - do not use innerHTMl
| async function displayData() { | ||
| await fetchCountriesData(); | ||
| if (countryFetchedData) { | ||
| analyzeMainContainer(countryFetchedData); |
There was a problem hiding this comment.
You can use guard clause for early return making the code cleaner :
async function displayData() {
await fetchCountriesData();
if (!countryFetchedData) {
console.error('Error displaying countries data');
return;
}
analyzeMainContainer(countryFetchedData);
getDropdownsRegions(countryFetchedData);
getSearchdata();
handleDropdownClick();
}| function displayCountryDetails() { | ||
| const countryInfo = document.querySelector('.country-details'); | ||
| const countryData = JSON.parse(localStorage.getItem('countryDetails')); | ||
|
|
There was a problem hiding this comment.
This for example could be a function coming from localsessionService and this is why this is a good practice. In your approach you will have to write that parsing logic again and again
2 participants
No description provided.