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

TTS Autoplay bug fix #56324

Merged
merged 29 commits into from Apr 11, 2024
Merged

TTS Autoplay bug fix #56324

merged 29 commits into from Apr 11, 2024

Conversation

juanmanzojr
Copy link
Contributor

@juanmanzojr juanmanzojr commented Feb 5, 2024

Creates a Queue for InlineAudios such as instructions and hints to prevent Text to Speech audio from playing all at once when Text to Speech is enabled in a course.

Before Audio Queue

Untitled.mov

After Audio Queue implementation

Screen.Recording.2024-04-01.at.12.35.07.PM.mov

Clicking hint while hint Audio is playing pauses hint audio, Clicking hint while another audio is playing adds to queue

Screen.Recording.2024-04-01.at.12.30.50.PM.mov

Links

Testing Story

Changes made to InlineAudioTest.js to include the AudioQueue context. All tests pass.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@juanmanzojr
Copy link
Contributor Author

This PR is a continuation of #55713, which was mis-closed by the Git LFS migration (#55759).

Previous Comments:

Previous Reviews:

  • carl-codeorg commented - Had one question about the shared_consts and one suggestion for a new test. Overall I think the queue was a great idea. Nice thinking!

@juanmanzojr juanmanzojr requested review from a team as code owners February 8, 2024 18:17
@cat5inthecradle cat5inthecradle removed request for a team March 27, 2024 13:38
@juanmanzojr juanmanzojr requested a review from a team April 2, 2024 21:12
@@ -652,6 +652,33 @@ module SharedConstants
}
).freeze

VOICES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Resurrecting my old comment from the pre-LFS PR: Why are we putting these into shared_constants.rb if none of these constants are referenced in Rails?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was because we had duplicated the VOICES in both JavaScript and Ruby (in dashboard/app/models/concerns/text_to_speech.rb) and that's what SharedConstants are for.

What is missing is using the SharedConstants version of VOICES in the TextToSpeech concern and removing it from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, makes total sense. We should probably open a ticket to do that, otherwise we haven't really improved the situation by moving them into shared_constants.

Copy link
Contributor

@carl-codeorg carl-codeorg left a comment

Choose a reason for hiding this comment

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

LGTM

@juanmanzojr juanmanzojr merged commit f1e81f0 into staging Apr 11, 2024
2 checks passed
@juanmanzojr juanmanzojr deleted the tts-autoplay-bug-fix branch April 11, 2024 16: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.

None yet

3 participants