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.

Sign up for GitHub

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

Jump to bottom

Add audio playback for feedback, hints, and solutions to learner view #4346

Merged
merged 19 commits into from
Jan 6, 2018
Merged

Add audio playback for feedback, hints, and solutions to learner view #4346

merged 19 commits into from
Jan 6, 2018

Conversation

tjiang11
Copy link
Contributor

@tjiang11 tjiang11 commented Jan 4, 2018 •
edited
Loading

WIP: This doesn't include hints and solutions yet. I'll finish this up after the hints refactor takes place, which I'll try to get around to reviewing later tonight. But this shouldn't take long once that gets in.

Note: I renamed AudioTranslationManagerService to AudioTranslationLanguageService and made a new AudioTranslationManagerService

seanlip
seanlip reviewed Jan 4, 2018
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thanks @tjiang11 -- I don't really have much to say (with regards to improvements)! Only main suggestion I have is to have one or more demo explorations with audio files for all the relevant components, for ease of testing in the future.

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js Outdated
_explorationLanguageCode).id;
oppia.factory('AudioTranslationManagerService', function() {
// Audio translations in the main conversation flow, such as those for
// content an feedback.
Copy link
Member

Choose a reason for hiding this comment

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

typo: and

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js Outdated
var _primaryHtmlForAutogeneratedAudio = null;

// Audio translations outside of the main conversation flow, such as
// those for hint and solutions.
Copy link
Member

Choose a reason for hiding this comment

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

typo: hints

@seanlip seanlip mentioned this pull request Jan 4, 2018
Closed
@anmolshkl
Copy link
Contributor

Hi @tjiang11, could you also please resolve the conflicts?

@codecov-io
Copy link

codecov-io commented Jan 5, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@34fbc9c). Click here to learn what that means.
The diff coverage is 63.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4346   +/-   ##
==========================================
  Coverage           ?   44.49%           
==========================================
  Files              ?      372           
  Lines              ?    22997           
  Branches           ?     3675           
==========================================
  Hits               ?    10232           
  Misses             ?    12765           
  Partials           ?        0
Impacted Files Coverage Δ
...ead/domain/exploration/ExplorationObjectFactory.js 58.46% <ø> (ø)
...ead/pages/exploration_player/TutorCardDirective.js 2.7% <0%> (ø)
...head/components/HintAndSolutionButtonsDirective.js 2% <0%> (ø)
.../exploration_player/HintAndSolutionModalService.js 2.94% <0%> (ø)
...es/exploration_player/ConversationSkinDirective.js 2.08% <0%> (ø)
...ev/head/pages/exploration_player/PlayerServices.js 0.98% <0%> (ø)
...head/pages/exploration_player/AudioBarDirective.js 0.84% <0%> (ø)
...v/head/pages/exploration_player/PlayerConstants.js 100% <100%> (ø)
.../pages/exploration_player/AudioPreloaderService.js 85.71% <100%> (ø)
...loration_player/AudioTranslationLanguageService.js 60.93% <60.93%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34fbc9c...599d7ee. Read the comment docs.

@tjiang11
Copy link
Contributor Author

tjiang11 commented Jan 5, 2018

@seanlip PTAL

@tjiang11
Copy link
Contributor Author

tjiang11 commented Jan 5, 2018

Just noticed some problems with the autogenerated audio, fixing now.

seanlip
seanlip approved these changes Jan 5, 2018
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks @tjiang11. LGTM. Just one question. After you get a couple of answers wrong and then press the "replay" button, shouldn't it read the content card too (not just the last feedback)?

@tjiang11
Copy link
Contributor Author

tjiang11 commented Jan 5, 2018

Oh, I think I forgot about that.. Sorry I'll fix it so that it always starts from content and reads content + last feedback (is that right?)

@seanlip
Copy link
Member

seanlip commented Jan 5, 2018

Actually let's just have it read the content when manually triggered. I think we decided that the last feedback was going to be read only as a one-off for now (when it appears). Otherwise it's hard to separate the content from the feedback aurally.

@seanlip
Copy link
Member

seanlip commented Jan 6, 2018

(Also please note merge conflicts)

@tjiang11
Copy link
Contributor Author

tjiang11 commented Jan 6, 2018

@seanlip PTAL

seanlip
seanlip approved these changes Jan 6, 2018
View reviewed changes
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

All good, I think. Thanks @tjiang11!

If you get to this in time, I suggest adding some comments to define "primary" and "secondary" audio translations in AudioTranslationManager.js. If you don't get to this in time, I'll merge it anyway.

Thanks!

@tjiang11 tjiang11 mentioned this pull request Jan 6, 2018
Merged
@seanlip seanlip merged commit 6adb3f9 into oppia:develop Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanlip seanlip seanlip approved these changes

Assignees

@tjiang11 tjiang11

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tjiang11 @anmolshkl @codecov-io @seanlip