Tomer #14
Open
Tomer #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…l to .country-details
…y contains the input and show the rellevant cards in the page
GilHeller
requested changes
Dec 19, 2024
index.html
Outdated
Comment on lines
58
to
63
| <li data-region="all" onclick="filterByRegion(event)">All</li> | ||
| <li data-region="africa" onclick="filterByRegion(event)">Africa</li> | ||
| <li data-region="americas" onclick="filterByRegion(event)">America</li> | ||
| <li data-region="asia" onclick="filterByRegion(event)"v>Asia</li> | ||
| <li data-region="europe" onclick="filterByRegion(event)">Europe</li> | ||
| <li data-region="oceania" onclick="filterByRegion(event)">Oceania</li> |
There was a problem hiding this comment.
consider to insert the li elements from the js code with map().
Imagine you have a large list of regions - we will want the computer to calculate the presented list and not to insert it one by one,
index.html
Outdated
Comment on lines
99
to
143
| <!-- | ||
|
|
||
| <a | ||
| href="#" | ||
| class="country scale-effect" | ||
| data-country-name="Åland Islands" | ||
| > | ||
| <div class="country-flag"> | ||
| <img src="https://flagcdn.com/ax.svg" alt="Åland Islands FLag" /> | ||
| </div> | ||
| <div class="country-info"> | ||
| <h2 class="country-title">Åland Islands</h2> | ||
| <ul class="country-brief"> | ||
| <li><strong>population: </strong>28875</li> | ||
| <li><strong>Region: </strong>Europe</li> | ||
| <li><strong>capital: </strong>Mariehamn</li> | ||
| </ul> | ||
| </div> | ||
| </a> | ||
| <a href="#" class="country scale-effect" data-country-name="Aruba"> | ||
| <div class="country-flag"> | ||
| <img src="https://flagcdn.com/aw.svg" alt="Aruba FLag" /> | ||
| </div> | ||
| <div class="country-info"> | ||
| <h2 class="country-title">Aruba</h2> | ||
| <ul class="country-brief"> | ||
| <li><strong>population: </strong>106766</li> | ||
| <li><strong>Region: </strong>Americas</li> | ||
| <li><strong>capital: </strong>Oranjestad</li> | ||
| </ul> | ||
| </div> | ||
| </a> | ||
| <a href="#" class="country scale-effect" data-country-name="Belize"> | ||
| <div class="country-flag"> | ||
| <img src="https://flagcdn.com/bz.svg" alt="Belize FLag" /> | ||
| </div> | ||
| <div class="country-info"> | ||
| <h2 class="country-title">Belize</h2> | ||
| <ul class="country-brief"> | ||
| <li><strong>population: </strong>397621</li> | ||
| <li><strong>Region: </strong>Americas</li> | ||
| <li><strong>capital: </strong>Belmopan</li> | ||
| </ul> | ||
| </div> | ||
| </a> --> |
JS/index.js
Outdated
Comment on lines
127
to
180
| // const containerCountries = document.querySelector(".countries-grid"); | ||
| // const countryCard = document.querySelector(".country.scale-effect"); | ||
| // const cards= document.querySelectorAll(".country"); | ||
|
|
||
| // fetch('./CountriesData.json') | ||
| // .then((res) => { | ||
| // if (!res.ok) { | ||
| // throw new Error | ||
| // (`HTTP error! Status: ${res.status}`); | ||
| // } | ||
| // return res.json(); | ||
| // }) | ||
| // .then((data) => { | ||
| // countries = data; | ||
| // console.log(data); | ||
| // duplicateCard(); | ||
| // innerTextIntoCardDetails(countries); | ||
| // }) | ||
| // .catch((error) => | ||
| // console.error("Unable to fetch data:", error)); | ||
|
|
||
| // const duplicateCard = () =>{ | ||
| // const lenArray = countries.length; | ||
|
|
||
| // for(i=1;i<lenArray;i++){ | ||
| // const cloneCard = countryCard.cloneNode(true); | ||
| // containerCountries.appendChild(cloneCard); | ||
| // } | ||
| // } | ||
| // const innerTextIntoCardDetails = (array) =>{ | ||
| // const cards= document.querySelectorAll(".country"); //take after the duplicate | ||
|
|
||
| // for(i=0;i<countries.length;i++){ | ||
| // const titleElement = cards[i].querySelector(".country-title"); | ||
| // const flagImage = cards[i].querySelector(".country-flag img"); //choost the img in the selector | ||
| // const countryInfo = cards[i].querySelectorAll(".country-brief li"); | ||
|
|
||
| // countryInfo.forEach((item)=>{ | ||
| // const label = item.querySelector("strong").textContent.trim().replace(":", ""); | ||
| // if(label==="population"){ | ||
| // item.innerHTML = countries[i].population; | ||
| // } | ||
| // else if(label==="Region"){ | ||
| // item.innerHTML = countries[i].region; | ||
| // } | ||
| // else if(label==="capital"){ | ||
| // item.innerHTML = countries[i].capital; | ||
| // } | ||
| // }) | ||
| // titleElement.innerHTML = countries[i].name; | ||
| // flagImage.src = countries[i].flag; | ||
| // flagImage.alt = `${countries[i].name}`; | ||
| // } | ||
| // } |
JS/index.js
Outdated
| console.log("enter"); | ||
| const currentColor = getComputedStyle(document.body).backgroundColor; | ||
|
|
||
| if (currentColor === "rgb(33, 37, 41)") { |
There was a problem hiding this comment.
consider to assign the hard-coded values: "rgb(33, 37, 41)" and "rgb(255, 255, 255)" to constant variables.
consider to create a new file for your application constants: const.js
Comment on lines
+71
to
+76
| const matchValues = (country, input) => { | ||
| return (country.name.toLowerCase().includes(input) || | ||
| country.population.toString().includes(input)) || | ||
| country.region.toLowerCase().includes(input) || | ||
| country.capital.toLowerCase().includes(input) | ||
| } |
There was a problem hiding this comment.
NICE!
Consider a case where the user enters " Israel " (with spaces and tabs before and after the word)
https://www.geeksforgeeks.org/string-strip-in-javascript/
JS/index.js
Outdated
Comment on lines
32
to
38
| titleElement.innerHTML = country.name; | ||
| flagImage.src = country.flag; | ||
| flagImage.alt = `${country.name}`; | ||
|
|
||
| countryInfo[0].innerHTML = `<strong>Population:</strong> ${country.population}`; | ||
| countryInfo[1].innerHTML = `<strong>Region:</strong> ${country.region}`; | ||
| countryInfo[2].innerHTML = `<strong>Capital:</strong> ${country.capital}`; |
There was a problem hiding this comment.
The usage of innerHTML is not recommended due to security vulnerabilities.
…hange from innerHtml in innerDetailsIntoCards func and more
|
i did the changes you asked from me and did commit on this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
i'm sending a pull request about the exercise. In the commits, you can see the acts I did.