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

Adjust content search when switching between transcripts #532

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

Dananji
Copy link
Collaborator

@Dananji Dananji commented Jun 21, 2024

Refinements to feature added in #502

This PR includes the following refinements to content search:

  • Use cached search results when switching between transcripts in the same Canvas without changing the query
  • Preserve search counts when switching to a transcript where search is disabled
  • Update search results counts properly when switching between transcripts
  • Clear search counts when current search is cleared
  • Display hit counts correctly when there are multiple hits in a single cue
  • General refactoring to parts of code related to transcript component

@Dananji Dananji marked this pull request as draft June 24, 2024 13:25
@Dananji Dananji marked this pull request as ready for review June 24, 2024 19:16
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

I did a little testing and search feels improved. I ran into a couple inconsistencies:

  • highlighting words within other words (e.g. you and your)
  • phrase searching correctly highlights but hit count is 0 (e.g. feeling better)

src/services/transcript-parser.js Outdated Show resolved Hide resolved
@Dananji Dananji marked this pull request as draft June 26, 2024 17:59
@Dananji Dananji marked this pull request as ready for review June 26, 2024 18:08
@Dananji Dananji marked this pull request as draft June 27, 2024 13:02
@Dananji Dananji marked this pull request as ready for review June 27, 2024 14:22
Copy link
Contributor

@masaball masaball left a comment

Choose a reason for hiding this comment

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

As far as I can tell this looks good.

src/services/transcript-parser.js Outdated Show resolved Hide resolved
Co-authored-by: Mason Ballengee <68433277+masaball@users.noreply.github.com>
@Dananji Dananji merged commit 0322404 into main Jun 27, 2024
2 checks passed
@Dananji Dananji deleted the refine-content-search branch June 27, 2024 15:59
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