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(frontend/controller): improve match highlighting + split up the search component #594

Merged
merged 12 commits into from
Oct 26, 2020

Conversation

saihaj
Copy link
Member

@saihaj saihaj commented Jul 16, 2020

Summary of PR

The Results function was very huge and it was getting quite confusing to navigate around. Idea here is to split up the search component so that results becomes its own component and all highlighting is contained there. Search component will pass down all the necessary props.

I tried bringing context down to results (where it makes way much more sense and then we don't have to pass so many things down) but was not possible because it breaks react hooks rules. Later I realized it is also not so great because we are mapping though the list of results we get from backend and that might be too much work each time to get context from the hook.

@Harjot1Singh you probably have some better way because this has reached very high levels of prop drilling.

Fixes

Fixes #567
Fixes #569

TTR

@saihaj - unsure
@Harjot1Singh - 2 hours

@saihaj saihaj requested a review from Harjot1Singh July 16, 2020 23:12
@saihaj
Copy link
Member Author

saihaj commented Jul 17, 2020

Now I have moved all the result related things to Result component. We pass down necessary props to Result component and it will take care of mapping through and highlighting of each search result. It is a first step towards simplifying the search component so that we can start working on #511.

@saihaj
Copy link
Member Author

saihaj commented Jul 17, 2020

@Harjot1Singh I need help, I messed up highlighting of a search results when keyboard keys are pressed to select an item.

Split search component so that results are their own component
this allows to use variables in scope rather than having to pass them down
Refactors Results into single result component
@Harjot1Singh Harjot1Singh requested a review from bhajneet October 26, 2020 02:38
@Harjot1Singh
Copy link
Member

Please check this still works well @bhajneet @saihaj.

I ended up fixing some search highlighting issues, and refactoring the logic into a separate file.

@saihaj
Copy link
Member Author

saihaj commented Oct 26, 2020

✅ LGTM
This also fixes #567 #569

@Harjot1Singh
Copy link
Member

This serves as a very good basis and the preliminary work required by #221, which will serve as one of our biggest performance improvements.

bhajneet
bhajneet previously approved these changes Oct 26, 2020
Copy link
Member

@bhajneet bhajneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Would have liked a little more narrative with comments. Working fine with tests of wildcard characters, full words, and all options in search (translit/translation/citation, etc.). LGTM!

app/frontend/src/Controller/Search/Result.js Outdated Show resolved Hide resolved
Harjot1Singh
Harjot1Singh previously approved these changes Oct 26, 2020
@Harjot1Singh Harjot1Singh changed the title refactor(frontend/controller): split up the search component fix(frontend/controller): improve match highlighting + split up the search component Oct 26, 2020
@Harjot1Singh Harjot1Singh merged commit 4b2c3b5 into shabados:dev Oct 26, 2020
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.

Last word repeating in search results Broken search results highlighter for matching hindi/urdu translits
3 participants