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 find in page to talk pages #4376

Merged
merged 18 commits into from Nov 4, 2022
Merged

Add find in page to talk pages #4376

merged 18 commits into from Nov 4, 2022

Conversation

staykids
Copy link
Contributor

Phabricator: https://phabricator.wikimedia.org/T319470 (partially)

Notes

Adds find in page to talk pages.

More refinement work in a forthcoming PR remains, including:

  • More precise scrolling to active search results when in a long blob of text
  • Dismissing the find in page bar when the user performs a competing action (like tapping to post a reply)

Test Steps

  1. Open a talk page and tap the find in page tab bar item
  2. Try searching for a term, then tapping the keyboard bar actions (next, previous, clear, etc.)
  3. Change themes and try again, confirming there are no render issues

@staykids staykids requested review from a team and tonisevener and removed request for a team October 27, 2022 21:02
@tonisevener tonisevener self-assigned this Oct 28, 2022
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

This is great to see! Nice job working with the existing search view.

I have a few actual code review comments, and then the rest are suggestions I tinkered with for getting the scrolling to work a bit more predictably. Let me know what you think!

@tonisevener tonisevener removed their assignment Oct 28, 2022
@staykids staykids self-assigned this Nov 2, 2022
@staykids
Copy link
Contributor Author

staykids commented Nov 2, 2022

@tonisevener Thanks for the great suggestions - updated and ready for another look!

@staykids staykids removed their assignment Nov 2, 2022
@tonisevener tonisevener self-assigned this Nov 3, 2022
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

Looking good! I had to adjust a little bit from some merge conflicts from main. We are now letting talk page items display that have no replies, so that caused some adjustments in the view models. main is also utilizing the removingInitialNewlineCharacters method in more places, which threw off some of the ranges for the active highlight.

I patched these up as best I could, but it could probably use more attention with the next PR. I'm going to go ahead and merge to get the current functionality in though.

I also noticed that it's no longer sluggish when I type, so perhaps the move away from reloadData improved that.

@tonisevener tonisevener removed their assignment Nov 3, 2022
@tonisevener tonisevener merged commit 9d3a5c9 into main Nov 4, 2022
@tonisevener tonisevener deleted the talk-pages/find-in-page-2 branch November 4, 2022 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants