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

Fix/1 sync people filter and search #7

Closed

Conversation

YLeight
Copy link
Contributor

@YLeight YLeight commented Jan 24, 2024

Handle searching and filtration together

Fix: #1

https://jam.dev/c/0c3e8498-5e22-43d5-b81c-a4a8f526af85

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from 2e48698 to e8138dc Compare January 24, 2024 00:20
@YLeight
Copy link
Contributor Author

YLeight commented Jan 24, 2024

Blocked by #4 & #6

@ecurrencyhodler
Copy link
Contributor

Video looks good. Sorry one more thing. We have reverted our backend api calls from api.people-test->people.test. So we no longer will require the "api" in front of the call.

Can you push another commit? I think that'll trigger our github actions workflows for testing. Also in the future, you can run the tests locally as well using: yarn run.

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from e8138dc to cfae158 Compare January 25, 2024 08:07
@YLeight
Copy link
Contributor Author

YLeight commented Jan 25, 2024

@ecurrencyhodler

@YLeight YLeight marked this pull request as draft January 25, 2024 08:24
@YLeight
Copy link
Contributor Author

YLeight commented Jan 25, 2024

Checking tests

@YLeight YLeight marked this pull request as ready for review January 25, 2024 10:52
@elraphty
Copy link
Collaborator

@YLeight fix Jest and Prettier issues

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from 423af48 to 83038fe Compare January 25, 2024 17:47
@ecurrencyhodler
Copy link
Contributor

All checks pass. @elraphty

@YLeight
Copy link
Contributor Author

YLeight commented Jan 26, 2024

I think is better to create new branch from new master as it was updated forcily

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from 9073d03 to d4cb4d4 Compare January 26, 2024 08:16
@YLeight
Copy link
Contributor Author

YLeight commented Jan 26, 2024

@ecurrencyhodler @elraphty And here we go again

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from d4cb4d4 to 767b6c4 Compare January 28, 2024 09:04
@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

@ecurrencyhodler @elraphty kindly reminder

@elraphty
Copy link
Collaborator

Great work @YLeight, it works well, but the user search by alias call is supposed to make a backend call so it can be a global search, and not an array search check getPeople() in https://github.com/stakwork/sphinx-tribes-frontend/blob/ec15331f75313cfb3dd6a672e4fd4ba480b52ca0/src/store/main.ts, calling the function with a search text is the way to go, thanks.

@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

Great work @YLeight, it works well, but the user search by alias call is supposed to make a backend call so it can be a global search, and not an array search check getPeople() in https://github.com/stakwork/sphinx-tribes-frontend/blob/ec15331f75313cfb3dd6a672e4fd4ba480b52ca0/src/store/main.ts, calling the function with a search text is the way to go, thanks.

Are you about this place, right?

@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

Or about this?

@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

@elraphty Or do you mean its would be better to rewrite old logic with usage of src/people/utils/filterPeople.ts util with real request, by passing all language filters as query params?

@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

@elraphty Or do you mean its would be better to rewrite old logic with usage of src/people/utils/filterPeople.ts util with real request, by passing all language filters as query params?

Seems like in this case the util function is no more needed, and I can remove is as a legacy code. To avoid not recommended usage in the future.

@elraphty Are we on the same page?

@elraphty
Copy link
Collaborator

@elraphty Or do you mean its would be better to rewrite old logic with usage of src/people/utils/filterPeople.ts util with real request, by passing all language filters as query params?

Seems like in this case the util function is no more needed, and I can remove is as a legacy code. To avoid not recommended usage in the future.

@elraphty Are we on the same page?

yeah, we don't need the util function for searching it will only filter 100 users, the people API endpoint already can search if a search query is passed, you can leave the util function for just the skills filter, we will also move the filter search to the backend later, but it is not related to this issue.

@YLeight
Copy link
Contributor Author

YLeight commented Jan 29, 2024

the people API endpoint already can search if a search query is passed

I've used getPeople method from main store, as I can see, this method get search query from ui store.

you can leave the util function for just the skills filter

I've got observed people field from main store which was updated by people setter from getPeople method
And I applied util filterPeople util function for skills filtration of current search result

src/hooks/usePeopleFilters.ts Outdated Show resolved Hide resolved
src/hooks/usePeopleFilters.ts Outdated Show resolved Hide resolved
@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from 9aa3405 to fa678a1 Compare February 1, 2024 17:14
@YLeight YLeight requested a review from elraphty February 1, 2024 17:23
@YLeight
Copy link
Contributor Author

YLeight commented Feb 1, 2024

@elraphty Tricky content test id on loading conditional rendering

Ready for review

As I've noticed on a meeting, it seems like languages query string should be persistent (stored in a main store)
Since after page refreshing, people data are got from the local storage, but not from the request result, it could lead inconsistent state of ui with empty filters and filtered people data from local storage as it was with search result in stakwork/sphinx-tribes#1437

Can check after people-test.sphinx.chat/people endpoing fix

P.S. Thanks for patience =) Was too excited)

@elraphty
Copy link
Collaborator

elraphty commented Feb 1, 2024

@elraphty Tricky content test id on loading conditional rendering

Ready for review

As I've noticed on a meeting, it seems like languages query string should be persistent (stored in a main store) Since after page refreshing, people data are got from the local storage, but not from the request result, it could lead inconsistent state of ui with empty filters and filtered people data from local storage as it was with search result in stakwork/sphinx-tribes#1437

Can check after people-test.sphinx.chat/people endpoing fix

P.S. Thanks for patience =) Was too excited)

@YLeight The endpoint is up and running https://people-test.sphinx.chat/people?search=Evan, can you confirm from your end that it works?

@elraphty
Copy link
Collaborator

elraphty commented Feb 1, 2024

@YLeight

Tested it, this and my findings from this Demo

https://www.loom.com/share/7dc68d5af5ab48c3a81382f770c18a6c

@YLeight
Copy link
Contributor Author

YLeight commented Feb 6, 2024

Seems like backend is again unavailable

@YLeight
Copy link
Contributor Author

YLeight commented Feb 6, 2024

It might help to also rebase and squash.

I do this every day already)

@YLeight
Copy link
Contributor Author

YLeight commented Feb 6, 2024

Awesome, languages now are also a part of url

Thanks to @elraphty

As discussd with @elraphty

I am going to make interactive rebase of the commit history
Make separate PR with husky fix

During this PR were fixed:

@YLeight YLeight marked this pull request as draft February 6, 2024 19:59
@ecurrencyhodler
Copy link
Contributor

Okay awesome! do you want to remove this PR from draft? I"m also going to increase your bounties due to the extra work you did. This bounty went from 300k->400k. https://community.sphinx.chat/bounty/1268 Thanks Yegor.

Hook incapsulate responsibility for filtering people list implicitly by search query (with mobx), and explicitly by coding languages (with react state)

Reates: stakwork#1
Add annotations related to filtering functionality by coding language's
`SearchTextInput` doesn't use this prop in any way.
It relys on value from store, copied to local react state
`SearchTextInput` doesn't sync local state with original store value
This reason any store updates (e.g. initialization from local storage) will be skipped without force re-render of it's parent component.
To provide consistent independent behaviour from any parent context add `useEffect` for synchronization
…people page and other routes"

This reverts commit cf9997c.
@YLeight
Copy link
Contributor Author

YLeight commented Feb 7, 2024

Today, this request https://people-test.sphinx.chat/people?page=1&resetPage=true&languages=Typescript,Lightning&search=ec&sortBy=last_login&limit=500 responds with incorrect data
Attached video:
https://jam.dev/c/74573792-2400-4918-b550-2bbae443208a

@YLeight YLeight force-pushed the fix/1-sync-people-filter-and-search branch from 1d4ce59 to 51cc006 Compare February 7, 2024 14:32
@YLeight
Copy link
Contributor Author

YLeight commented Feb 7, 2024

Implemented uploading synced with filter and search result

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.

Filter and search on People page isn't synced
3 participants