Skip to content

Find and replace #1

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

Closed
wants to merge 6 commits into from
Closed

Find and replace #1

wants to merge 6 commits into from

Conversation

RajWorking
Copy link
Owner

Created a UI functionality that allows searching for words and replacing them, either one by one or all together.
The find button allows searching a word in the transcript and highlighting it.
Clear remove the highlighted found words.

@adamjonas
Copy link
Collaborator

Cool. Looks like you've got a start for this. Some feedback below:

  • The replace and replace all functionality seem to be the same. I don't see a way to do individual replacements.
  • The modal requires a second click on the button to disappear, you should be able to click outside of the dropdown in order for it to close
  • This is fine for a first pass on the find button but immediate highlighting based on what has already been typed (like in the browser) is likely the smoother implementation of this
  • once I do a replace/replace all, all the text is highlighted and I no longer can get the read-along highlighting

@RajWorking
Copy link
Owner Author

ACK. Will get on these.

    - can click outside dropdown to close
    - replace and replace all functionality works
    - read-along highlighting unaffected
@RajWorking
Copy link
Owner Author

I have fixed the above issues. Please review the commits.

@adamjonas
Copy link
Collaborator

I'm ok with the choices, but replacing the speaker labels is no longer possible. Is there going to be a follow-up to address updating the speaker tags?

@RajWorking
Copy link
Owner Author

You can update the speaker labels easily now. Currently, they have a hardcoded transcript text so I have fixed the bug in that. There is still an issue with loading transcripts directly from JSON (maybe I can pick that later on).

@RajWorking RajWorking force-pushed the find-and-replace branch 3 times, most recently from 6d75381 to 6fef865 Compare February 8, 2023 06:28
@adamjonas
Copy link
Collaborator

  1. Enter key doesn't work for either button, which would be expected behavior.

  2. Cannot replace a previously replaced text.

Steps to repro in video here:

Screen.Recording.2023-02-10.at.11.20.28.AM.mov

@RajWorking
Copy link
Owner Author

RajWorking commented Feb 12, 2023

Regarding 1:
Once you have typed in the "new text", you can use tab to go to the replace and replaceAll buttons. Enter will work then.
I am not sure what is expected if one presses enter just after typing the "new text"? Is it simply executing the Replace button functionality (not ReplaceAll). So basically pressing "Enter" in textfield is same as pressing "Tab Enter"

@RajWorking
Copy link
Owner Author

RajWorking commented Feb 12, 2023

Regarding 2:
Each word is characterized by its starting time and duration. So when you replace some text with a phrase, that entire phrase is seen to be having a single start time and duration. If need be, you can find and replace the entire phrase at once instead of trying to replace a part of it.
So in your video, replace "scan" with "won't scan", then maybe replace "won't scan" with "will scan".

@adamjonas
Copy link
Collaborator

Is it simply executing the Replace button functionality (not ReplaceAll)

yes, this is the expected behavior similar to functionality that you might find in a google doc or a text editor.

@adamjonas
Copy link
Collaborator

So in your video, replace "scan" with "won't scan", then maybe replace "won't scan" with "will scan".

This isn't possible because "new" text isn't highlighted upon replacement.

@RajWorking
Copy link
Owner Author

I believe that I should continue working on this functionality after the VTT transcript format has been completed. There might some changes to the JS code so I will resume working on this later

@RajWorking RajWorking closed this Feb 21, 2023
@adamjonas
Copy link
Collaborator

I think you should merge what you can so it can be used upstream.

@RajWorking RajWorking reopened this Feb 22, 2023
@RajWorking RajWorking closed this Feb 22, 2023
@RajWorking
Copy link
Owner Author

I have opened a PR directly to upstream.
#41

@adamjonas
Copy link
Collaborator

Is this PR meant to be open though?

@RajWorking
Copy link
Owner Author

No, if needed I will open a new PR later on.

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.

2 participants