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.
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 #457
base: main
Are you sure you want to change the base?
Friends app #457
Changes from 2 commits
212e0dd
83bf7c5
0f91fda
d15ac84
7fe11b6
9b4cd4d
354d014
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 😉
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 would be honest, but i didnt find the way how to delete this displayCards() from this piece of code, so i just renamed function. I was trying to put this displayCards() in document.addEventListener('DOMContentLoaded', function () , but it didnt work...
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.
Tip - you should use
then
in special place ;)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.
Yep, this I understood, but where:)
Ok, I will try to figure out!
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 special place" 😄
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.
Ok, I think I found out how to do it!
First of all - I tried to use one more promise inside of
document.addEventListener('DOMContentLoaded', function (){...}
, but it didnt work, i dont know why. Then I decided to useasync
and seems it worked!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.
displayList
?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.
Also, please check this comment #449 (comment)
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.
Thank you!
Yes, really, i dont need this 'assigning '!
Ok, I will read this comments.
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'm sorry, Did I understand correctly - I should remake this part of code (this switch is too long and it's not "Single responsibility"):
if (radio) {
switch (radio.id) {
case 'genderAll':
displayList = persons;
break;
case 'genderMale':
displayList = persons.filter(el => el.gender === 'male');
break;
case 'genderFemale':
displayList = persons.filter(el => el.gender === 'female');
break;
case 'nameAsc':
displayList.sort((b, a) => a.name.last > b.name.last ? -1 : 1);
break;
case 'nameDesc':
displayList.sort((b, a) => a.name.last < b.name.last ? -1 : 1);
break;
case 'ageLow':
displayList.sort((a, b) => a.dob.age - b.dob.age);
break;
case 'ageHigh':
displayList.sort((b, a) => a.dob.age - b.dob.age);
break;
}
With adding functions for sorting.
BTW, Should I delete "long switch" and make few different "addEventListneres" for each case of switch or it will be very ig load on site? (sorry may be for stupid questions, but i just want to understand)
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.
change
event. Or you can separate listeners between fieldsets. Up to youThere 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.
Huh! Unbelievable, but seems I did it!:)
I dodnt know why, but this refactoring took me quit long time.
Basically, I think. It because I start fixing everything at one time... it was big mistake - nothing work and could understand why. So, then I started do it step-by-step, like You teaching us:) Refactored sort logic, then long switch.
Sorry, may be It's not necessary to write it here, but whom else can i write it;))
Ok, so now code looks better, I hope:)