-
-
Notifications
You must be signed in to change notification settings - Fork 185
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
Friends app #95
Friends app #95
Conversation
Hey! Congratulations on your PR! 😎😎😎 Please, be sure you haven't followed common mistakes. Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Great work for the first iteration. Let's fix some code.
For starters, you have a bug. Your sorts and filters don't persist. If you sort by name and then filter, sorting disappears. If you filter by a certain name and then filter by gender, it shows all women/men instead of only those that have the filtered name. Your right side of the app should always represent the results of the whole form state, not only the last clicked filter/sorting.
Secondly, from the UX perspective, your sorting should be displayed as radio buttons and it should be visible what sorting is selected at the current time.
Also, please check comments below.
@@ -0,0 +1,177 @@ | |||
const url = 'https://randomuser.me/api/?results=12'; | |||
|
|||
let aToZLast = document.querySelector('.sortedAtoZLast'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You almost exclusively use let
variables while most of them should be const
. Please review your code and change them.
const url = 'https://randomuser.me/api/?results=12'; | ||
|
||
let aToZLast = document.querySelector('.sortedAtoZLast'); | ||
let zToALast = document.querySelector('.sortedZtoALast'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of querySelector
s here. Instead, you can just get your form
element and then access any field of the form at any time by its name
.
So for example, you can have a couple of radiobuttons that share the same name
, let's say "gender". Each of them should have a value
attribute, like male
and female
for example.
So then you can just go
const form = document.querySelector("#myForm");
console.log(form.gender) // this should return male\female based on checked radiobutton
resArr.push(arr[i]); | ||
} | ||
basicArr1 = resultArr.slice(); | ||
// basicArr2 = JSON.parse(JSON.stringify(resultArr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code from the final version.
let minNumber = document.querySelector('#minNumber'); | ||
let maxNumber = document.querySelector('#maxNumber'); | ||
|
||
let basicArr1 = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basicArr1
doesn't seem like a correct name. You should avoid using data type in the name of the variables, it indicates that the name is not descriptive enough. Like resultArr
could just be friends
or users
. Plural name assumes that this is an array by itself.
} | ||
|
||
|
||
function saveUsers(arr, resArr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this function do, exactly?
It seems like it just copies the whole arr
to resArr
and then does the same for resultArr
and basicArr1
. It's not readable and seems like it doesn't make any sense at all. You can just save users right where you get your response from API.
|
||
async function getResponse(url) { | ||
let response = await fetch(url); | ||
let friendsArr = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even ar Arr, it's an object that has an array in the results
field.
// basicArr2 = JSON.parse(JSON.stringify(resultArr)); | ||
} | ||
|
||
function renderItem(item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading name, this function doesn't render
anything, it returns an HTML code for an element.
`; | ||
} | ||
|
||
aToZLast.addEventListener('click', sortedAtoZLast); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid using a lot of event listeners. This task can be done with just one that is placed on the whole form.
The bug still persists. Write something in the search field, and make sure a user or several of them are found by the name. Then, click different sorts and filters. The user list starts to ignore what's written in the search field. And it shouldn't. As I said, at any point in time, the list of users on the right should represent the state of the form on the left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that GitHub is still pending my review, so I'm going to post it here as a review. I saw your changes, but there are still bugs. Please make a lot of testing with your form. Sometimes First Name sorting just stops working, when you change age it doesn't really filter anything. And why is the label for age limits say Age 10 - 100
? You can change it for whatever, not only 10\100? :)
Just some advice, maybe change your name filtration to filter through both first and last names? Have you seen anywhere separate filtrations for name and surname? You can just stitch them to one string and filter them like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, nice work. Let's improve it.
You have some trouble with setting clear responsibilities for your functions. If you have a function that says filterUsersByName
, it should receive an array to filter and what to search for as parameters and return a filtered array in the end. And that's it. You then do anything you want with this array - sort it, render it, whatever, but this filterUsersByName
function should not care, it has done its work already. You can make a separate function for rendering and just pass data to it. You have a lot of functions that do some action, like sorting AND they also render to DOM. Such approach invites mistakes and reduces code readability.
</form> | ||
|
||
<form class="asideBlock__items" name="minMax"> | ||
<p>Age 10 - 100</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it exactly 10 - 100
?
console.log(err); | ||
resetPage(); | ||
} | ||
let jsonData = await response.json(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
let isSearch = false; | ||
|
||
async function getResponse(url) { | ||
let response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need an outside variable for response
. Do the .json()
thing right in the try
block. You have a faulty logic here, you're getting data from a response outside of try
in any scenario, even if the data isn't there because you ignore the catch
block. The only reason your program doesn't crash is that if there is an error you just reset your page, but let's not rely on that. If there is no data - don't extract it from the response.
renderAllItemsToPage(friends); | ||
} | ||
|
||
function creatingDivItem(item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What div
item? What is the content of this item? Use more descriptive names, please.
`; | ||
} | ||
|
||
form.last.addEventListener('click', function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't rely on the fact that your form is the last one on the page. Reason: there are a lot of browser extensions that modify the dom, for example, Grammarly adds its script to the end of DOM to work. That is why we don't style by tags and don't do JS by tags - because you never know what could go wrong with the DOM structure. Give your form an ID and do a querlySelector
.
} | ||
} | ||
|
||
function renderMale() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your renderMale
, renderFemale
and renderAll
functions are very similar. Please combine them.
findByAge(); | ||
} | ||
|
||
function findByAge() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of JS, find
means 1 result and filter
means multiple results.
form.search.search.addEventListener('input', searchFunc); | ||
|
||
function searchFunc() { | ||
isSearch = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a variable to track if there is something in the input. Call this function every time something changes, just don't do anything here if the input is empty.
isSearch = true; | ||
let val = form.search.search.value.toLowerCase(); | ||
if (val && val.length > 0) { | ||
content.innerHTML = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here? It's a searching function, it should return an array of users that match search input, not do things in the DOM.
|
||
content.innerHTML = ''; | ||
|
||
let acc = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a string of elements, it would be better to use the map
method and create an array of elements, just join them before rendering. This would allow you to manipulate this array in the future if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a bug now, if you select any gender, even the one that is already selected your sorting drops. Also, what happened to the name search and age filtration?
</head> | ||
<body> | ||
<div class="container"> | ||
<div class="header"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you go away from semantic header
and footer
tags?
const data = (url) => fetch(url) | ||
.then(handleErrors) | ||
.then(res => res.json()) | ||
.then(res => res.results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting a property from an object is not an asynchronous task, you don't need another .then
after this one.
@@ -0,0 +1,87 @@ | |||
const url = 'https://randomuser.me/api/?results=12'; | |||
const content = document.querySelector('.articleBlock'); | |||
const form = document.forms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have 2 forms? You should use 1 form and get it by id or something, don't rely on document.forms
.
|
||
switch(id) { | ||
case 'ageMore': | ||
friendsCopy = friendsCopy.sort((a, b) => a.dob.age - b.dob.age); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the functions that you use for sorting. They are identical, except you change parameter order. Maybe abstract it to not repeat yourself.
case 'lastZToA': | ||
friendsCopy = friendsCopy.sort((a, b) => a.name.last !== b.name.last ? a.name.last > b.name.last ? -1 : 1 : 0); | ||
break; | ||
case 'male': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is filtering, it should not belong to the sorting function.
content.innerHTML = acc; | ||
} | ||
|
||
data(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add empty lines at the end of all files.
…d filterBySearch, add init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check comments below.
</div> | ||
</form> | ||
|
||
<form name="gender" id="genderForm"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're still using separate forms. It makes your code more complicated in the JS. Why not tie it up in one form?
} | ||
} | ||
|
||
function filterBy(id, usersToFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the only id you have there is somehow connected with gender, so filterBy
is actually filterByGender
.
} | ||
|
||
function filterBy(id, usersToFilter) { | ||
switch(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can shorten this switch like this:
switch(id) {
case 'male':
case 'female':
usersToFilter = userstoFilter.filter(item => item.gender === id);
break;
case 'all':
...
}
usersToFilter = usersToFilter.filter(item => item.gender === 'female'); | ||
break; | ||
case 'all': | ||
usersToFilter = [...friends]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, it would be more readable to create a new variable, something like filteredUsers
in the beginning of this function to store the returned result. Secondly, if you're getting this from friends
what's the point of passing some array in the function argument?
return a.dob.age - b.dob.age; | ||
} | ||
|
||
function sortAlph(a, b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sortByName
? And sortByAge
let friends = []; | ||
let friendsCopy = []; | ||
|
||
const data = (url) => fetch(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getData
?
Great work! |
Friends App
Demo |
Code
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.