friends-app #731
friends-app #731
Conversation
|
Hey! Congratulations on your PR! 😎😎😎 Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code. Go through the requirements/most common mistakes linked below and fix the code as appropriate. If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat. When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly. 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. 😒 Please, make sure you haven't made common mistakes. In addition to the bot's suggestions above, please post a link to the app demo in Students' chat, collect feedback and fix bugs if any based on that. When user tests are done post here a comment on that. Universal recommendations:
By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
There was a problem hiding this comment.
Hi! I would like to request some changes without looking at the code yet since you have some strange behaviors in your project. First of all, pressing all gender seems to reverse the array of displayed users for some reason. Secondly, if you write a name in the search and then click any of the gender options - it clears out the search input (which it should not do).
Also, please include your other code files, not only the JS one.
|
Made edits. Also added other project files |
There was a problem hiding this comment.
Hi, sorry for the long delay.
Nice work, but there's a lot to fix and improve here. I'm not going to do a very thorough review, rather guide your thinking to another path and we'll work it out from there. Please follow comments in the code.
Besides, found a couple of bugs/rough edges in your work, make sure to fix them too:
- You can only search by users' first names in your work. Last names should be included too.
- In mobile version, I believe you set your menu to hide every time users changes something. This isn't a good place for such behaviour because:
- This isn't a navigation menu where you would like to close it after, for example, navigating to another page. This is a complicated "settings"-like menu, where user possibly would like to change several inputs at once before turning to content.
- This behaviour introduced a bug, where the menu would close each time you type a button in the search input, what makes it practically unusable.
Also, please include all code files that you worked with, not only js. html and css could still contain some mistakes and potential room for improvements.
| @@ -0,0 +1,8 @@ | |||
| let URL_API = 'https://randomuser.me/api/?results=50&nat=us,gb,ua&inc=gender,name,email,dob,phone,picture'; | |||
| let data = []; | ||
| let response = []; | ||
|
|
||
| const createCard = (data2) => { |
There was a problem hiding this comment.
To avoid numerating variable names like this you should give them more specific names. Data is cool and general, but barely helps you in development. What kind of data is it? Is it a user? Maybe several users? You should know what to expect from the data by the name here. And the global data on the 6th line is too general too. I can see it is an array, so it means users, right?
| let response = []; | ||
|
|
||
| const createCard = (data2) => { | ||
| const { name, gender, email, phone, picture, dob } = data2; |
There was a problem hiding this comment.
You could've done this where the function get's it's arguments. Like this:
const createCard = ({ name, gender, ... }) => {So no need in the data2 variable at all.
| let phones = Number(phone.replace(/\D+/g,'')); | ||
| if (phones.toString().length === 6) { | ||
| phones = '0000' + phones | ||
| } else if (phones.toString().length === 7) { | ||
| phones = '000' + phones | ||
| } else if (phones.toString().length === 8) { | ||
| phones = '00' + phones | ||
| } else if (phones.toString().length === 9) { | ||
| phones = '0' + phones | ||
| } else if (phones.toString().length === 11 || phones.toString().length === 12 || phones.toString().length === 13) { | ||
| phones = phones.toString().replace(phones.toString(), phones.toString().slice(-10)) | ||
| } | ||
| let result = phones.toString().replace(/^(.{3})(.{3})(.{2})(.*)$/, "($1) $2-$3-$4"); |
There was a problem hiding this comment.
This seems very complicated and while I understand what is your goal here, it looks ugly here. Maybe just separate it to it's own function like, preparePhoneNumber or something like it.
There was a problem hiding this comment.
moved to a separate function
| const sortAge = document.querySelector('.sort_by_age'); | ||
| const sortListAge = document.querySelector('.sorts_list_age'); | ||
| const sortIdAge = document.querySelector('#sort_by_age'); | ||
| const sortName = document.querySelector('.sort_by_name'); | ||
| const sortListName = document.querySelector('.sorts_list_name'); | ||
| const sortIdName = document.querySelector('#sort_by_name'); | ||
| const filterIdGender = document.querySelector('#filters_gender'); | ||
| const inputSearch = document.querySelector('#search_friends'); | ||
| const reset = document.querySelector('.reset_button'); | ||
| const inputAll = document.querySelector('#all'); |
There was a problem hiding this comment.
That's a lot of element picking. Remember that if you just wrap any inputs in a form, you can get this form with a query selector once and then access it's inputs' values any time you want, like with an object. Just remember to give them names.
There was a problem hiding this comment.
did not understand what to do. move to a separate file, into a function and export the names of the constants?
|
|
||
| let genders = Array.from(document.getElementsByName("gender")).find(r => r.checked).value; | ||
|
|
||
| sortAge.addEventListener('click', () => { |
There was a problem hiding this comment.
You also have a lot of event listeners. Ideally you can do the whole thing with only 1 event listener. Just place it on the form, track the "input" event and work with data in the form every time the user changes something.
|
|
||
| const sortByAge = () => { | ||
| if (inputSearch.value.length !== 0) { | ||
| const fByG = filterByGender() |
There was a problem hiding this comment.
Use fuller names, no need to shorten so much.
Also, what is filterByGender() doing here? I believe current function name is sortByAge. Your function should follow the single responsobility principle. Sorting function should not invoke filtering function. It shouldn't do anything besides sorting at all.
| if (target.classList.contains('sorts_options')) { | ||
| sortName.textContent = target.textContent; | ||
| sortIdName.value = target.dataset.sort; | ||
| console.log(sortIdName.value); |
There was a problem hiding this comment.
Don't leave console.logs in the finished code.
| renderCards(data) | ||
| }; | ||
|
|
||
| reset.addEventListener('click', e => { |
There was a problem hiding this comment.
honored Google, applied
|
fixed bugs and comments |
|
error correction |
friends-app
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.