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

Add comment search #887

Merged
merged 9 commits into from
Nov 25, 2023
Merged

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Nov 9, 2023

Pull Request Description

This PR introduces a basic comment text search within a post. There is a new FAB button from which you can initiate a search and enter a search term. After that, if there are any matches, the first matching comment will be highlighted. At that point, the main FAB button will become a "next result" button, allowing you to cycle through all the results. Then you can expand the FAB to cancel the search using the same button that initiated it.

Limitations

  • We can currently only scroll to top-level comments, so if there is a nested result, it might not be obvious (but it will be highlighted). This is a general problem and not new here.
  • With these changes, we are not specifically highlighting the matched word/phrase, only the whole comment.

Notes

  • I am using the newlyCreatedCommentId property of the PostState to highlight the search results. Since this property is really only used for highlighting, it could be renamed to a more general highlightedCommendId or similar. But that would be more of a refactoring task for the future.
  • I did ensure that, if cycling through results causes additional paging/fetching, we will stay in search mode.
  • Performing any other action (vote, save, etc.) will take us out of search mode.

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

qemu-system-x86_64_iz4ujnFOLO.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Collaborator

@ggichure ggichure left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

This looks really awesome! Thanks for taking this on, it'll definitely be useful to have a search function for comments.

I did ensure that, if cycling through results causes additional paging/fetching, we will stay in search mode.

Just for a bit of clarification here, if additional fetching happens, will the search results be updated (e.g., if the additional comment fetch retrieves a comment with the given search term firefox, will that new comment will be added into the results of the existing search)?

@micahmo
Copy link
Member Author

micahmo commented Nov 24, 2023

Just for a bit of clarification here, if additional fetching happens, will the search results be updated (e.g., if the additional comment fetch retrieves a comment with the given search term firefox, will that new comment will be added into the results of the existing search)?

Yes exactly. You can't tell in the video because the fetching happens off-screen, but at one point the "next result" button puts us within the threshold of fetching more comments. Some of those new comments contain the search term, so pressing "next result" again goes to the new one rather than back to the top.

@hjiangsu
Copy link
Member

Some of those new comments contain the search term, so pressing "next result" again goes to the new one rather than back to the top.

Awesome! Let me know if this is all good to be merged 😄

@micahmo
Copy link
Member Author

micahmo commented Nov 25, 2023

Should be good!

@hjiangsu hjiangsu merged commit b4be7d2 into thunder-app:develop Nov 25, 2023
1 check passed
@micahmo micahmo deleted the feature/comment-search branch November 27, 2023 12:41
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.

3 participants