Skip to content

Friends App by natashafir#465

Open
natashafir wants to merge 2 commits intokottans:mainfrom
natashafir:friends-app
Open

Friends App by natashafir#465
natashafir wants to merge 2 commits intokottans:mainfrom
natashafir:friends-app

Conversation

@natashafir
Copy link
Contributor

Friends App

Demo
Code base

The code is submitted in a dedicated feature branch.
Please, review.

@zonzujiro zonzujiro self-assigned this Feb 27, 2021
@zonzujiro zonzujiro added the Friends App Task 15: Friends App label Feb 27, 2021
@natashafir natashafir requested a review from zonzujiro February 27, 2021 14:25

function initialApp() {
fetch(URL)
.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.

You should handle fetch response status - https://www.tjvantoll.com/2015/09/13/fetch-and-errors/

const input = document.querySelector('input');

function initialApp() {
fetch(URL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should fetch data and handle requests in a separate function.

Comment on lines +53 to +79
function getSortedFriends(dataToSort, choosedRadio) {
if (choosedRadio == 'old-first' || choosedRadio == 'young-first'){
dataToSort.sort(function(x, y){
return x.age - y.age;
});
if(choosedRadio == 'old-first'){
dataToSort.reverse();
}
} else if (choosedRadio == 'name-AZ' || choosedRadio == 'name-ZA'){
dataToSort.sort(function(x, y){
let a = x.name.toUpperCase(),
b = y.name.toUpperCase();
return a == b ? 0 : a > b ? 1 : -1;
});
if (choosedRadio == 'name-ZA'){
dataToSort.reverse();
}
} else if (choosedRadio == 'all' || choosedRadio == 'female' || choosedRadio == 'male'){
filterBy = choosedRadio;
}

if (filterBy === 'all') {
return dataToSort;
} else {
return dataToSort.filter(element => element.gender === filterBy);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single responsibility - your functions should do only one job. As an example function in which you fetch users should only fetch them and not render, transform or process them in other ways. All these things should be done in another place, outside your function.

The same applied to the sort function, which usually does all types of sorting 😉

So, simplify this function

CARDS.innerHTML = '';
let cards = '';
item.forEach(elem => {
cards +=createCardItem(elem);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this function is redundant. Just put string here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do, app doesn't work. Or I don’t understand what you meant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please show an example of code how you doing this.

@OleksiyRudenko OleksiyRudenko added the stale Stale for 15 days or longer label Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Friends App Task 15: Friends App stale Stale for 15 days or longer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants