Skip to content
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

Bug fix: add local sorting if location filter is active #98

Merged
merged 5 commits into from
Mar 21, 2020

Conversation

janikga
Copy link
Collaborator

@janikga janikga commented Mar 21, 2020

@janikga janikga self-assigned this Mar 21, 2020
@kenodressel
Copy link
Collaborator

Could we solve this natively without an additional dependency? For example https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/sort could work. An example implementation of this could be found in https://github.com/florianschmidt1994/quarantaenehelden-firebase-functions/blob/master/functions/index.js#L103

@mauriceackel
Copy link
Member

+1 for doing it with array sort

src/components/FilteredList.jsx Outdated Show resolved Hide resolved
@janikga
Copy link
Collaborator Author

janikga commented Mar 21, 2020

@kenodressel @Maurice22 updated to use native Array.sort(). Please let me know if I should further refactor the logic, as requested by @Maurice22 or if we can leave it as-is for now!

@janikga
Copy link
Collaborator Author

janikga commented Mar 21, 2020

Just discovered location is available, will refactor!

@janikga
Copy link
Collaborator Author

janikga commented Mar 21, 2020

done

@mauriceackel
Copy link
Member

The issue with those variables is that they are set asynchronously by react. I need to check if location will always be available when we need it. I think I will refactor the code in this branch before continuing with the review.

@janikga
Copy link
Collaborator Author

janikga commented Mar 21, 2020

I'm pretty sure it is always available (assumption based on my testing), but feel free to test yourself :)

@kenodressel
Copy link
Collaborator

I think location should always be there. At least with the default value (empty string). So I am fine with this state.

@kenodressel kenodressel self-requested a review March 21, 2020 16:04
@mauriceackel
Copy link
Member

Alright, I added issue #100 for future refactoring.

@mauriceackel mauriceackel merged commit 17cb21a into develop Mar 21, 2020
@janikga janikga deleted the add-local-sorting-if-location-filter-active branch March 21, 2020 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamp sorting not working while searching
3 participants