Learner audio updates #3705

tjiang11 · 2017-08-05T04:56:29Z

Address some of the learner components of #3704.

Gray out the audio button when audio not available and show tooltip.
Warn the user when audio translation is flagged as needing update.

…slation flagged

seanlip

Thanks @tjiang11 -- looks great! A few small comments.

seanlip · 2017-08-05T05:02:20Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

+            return Boolean(getCurrentAudioTranslation());
+          };
+
+          $scope.isCurrentAudioTranslationNeedingUpdate = function() {


nit: doesCurrentAudioTranslationNeedUpdate

seanlip · 2017-08-05T05:03:29Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

+            return getCurrentAudioTranslation().needsUpdate;
+          };
+
+          $scope.audioNotAvailableMessage = 'Not available in ' + 


Think this would work for i18n of the tooltip text? angular-translate/angular-translate#163 (comment)

If we want to do that, maybe maintain $scope.currentAudioLanguage instead of retrieving it each time, and update it on-change. If all this is not straightforward, though, feel free to punt on it (but please file an issue).

tried this out, but ptal

seanlip · 2017-08-05T05:05:04Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

@@ -73,15 +91,10 @@ oppia.directive('audioControls', [
          };

          var loadAndPlayAudioTranslation = function() {
-            var currentAudioLanguageCode =
-              AudioTranslationManagerService.getCurrentAudioLanguageCode();
-
            // TODO(tjiang11): If audio translation is not available


Is this TODO still needed?

seanlip · 2017-08-05T05:06:35Z

core/templates/dev/head/pages/exploration_player/audio_controls_directive.html

+     tooltip="<[!isAudioAvailableInCurrentLanguage() ? audioNotAvailableMessage : '']>"
+     tooltip-append-to-body="true"></i>
+</div>
+<span ng-if="extraAudioControlsAreShown && isAudioAvailableInCurrentLanguage() && isCurrentAudioTranslationNeedingUpdate()" class="audio-message" translate="I18N_PLAYER_AUDIO_MIGHT_NOT_MATCH_TEXT"></span>

 <style>
  .audio-control-button-icon {


Prefix all these rules with audio-controls, since that's the name of the tag. Prevents the CSS from "leaking out" into other parts of the app.

seanlip · 2017-08-05T05:07:15Z

core/templates/dev/head/pages/exploration_player/audio_controls_directive.html

+  }
+
+  .audio-message {
+    float:right;


nit: add space after colon

seanlip · 2017-08-05T05:08:06Z

assets/i18n/en.json

@@ -306,6 +306,7 @@
    "I18N_MODAL_CANCEL_BUTTON": "Cancel",
    "I18N_ONE_SUBSCRIBER_TEXT": "You have 1 subscriber.",
    "I18N_PLAYER_AUDIO_LANGUAGE": "Language",
+    "I18N_PLAYER_AUDIO_MIGHT_NOT_MATCH_TEXT": "Audio might not match text",


Maybe "Audio might not fully match text"? (Otherwise it seems to raise the question of why we're putting a completely non-matching audio track there at all.)

seanlip · 2017-08-05T05:23:04Z

Also, the UI looks good to me, thanks @tjiang11!

codecov-io · 2017-08-05T05:55:37Z

Codecov Report

Merging #3705 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3705      +/-   ##
===========================================
- Coverage    45.07%   45.04%   -0.03%     
===========================================
  Files          264      264              
  Lines        20408    20421      +13     
  Branches      3173     3173              
===========================================
  Hits          9199     9199              
- Misses       11209    11222      +13

Impacted Files	Coverage Δ
...ploration_player/AudioTranslationManagerService.js	`2.56% <0%> (-0.22%)`	⬇️
...pages/exploration_player/AudioControlsDirective.js	`2.5% <0%> (-0.84%)`	⬇️

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 efd0d63...d52bda0. Read the comment docs.

seanlip

Thanks @tjiang11! LGTM, only one note to address before merge.

seanlip · 2017-08-05T18:55:51Z

core/templates/dev/head/pages/exploration_player/audio_controls_directive.html

-     ng-class="{'fa-volume-off': !AudioPlayerService.isPlaying(), 'fa-volume-up': AudioPlayerService.isPlaying(), 'audio-not-available': !isAudioAvailableInCurrentLanguage()}"
-     tooltip="<[!isAudioAvailableInCurrentLanguage() ? audioNotAvailableMessage : '']>"
+     ng-class="{'fa-volume-off': !AudioPlayerService.isPlaying(), 'fa-volume-up': AudioPlayerService.isPlaying(), 'audio-controls-audio-not-available': !isAudioAvailableInCurrentLanguage()}"
+     tooltip="<[!isAudioAvailableInCurrentLanguage() ? ('I18N_PLAYER_AUDIO_NOT_AVAILABLE_IN' | translate) + ' ' + currentAudioLanguageDescription : '']>"


Any way you can make "currentAudioLanguageDescription" a parameter of the I18N thing, similar to how I18N_FORMS_TYPE_NUMBER_AT_LEAST handles minValue? In other languages, currentAudioLanguageDescription may not always appear at the end.

If not, let's just not i18n it for now. I'm not sure how to do this sort of thing in tooltips, it might be the first time we've had to handle it.

Wouldn't we have to define translations for every audio language we have then?

Or is it okay to just leave the languages themselves in English

Language descriptions will actually be in that language itself. So it's OK to just use that description directly.

Btw backend tests seem to be failing...

* upstream/develop: (39 commits) Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717) Fix oppia#2998 implement collection skills update commands (oppia#3710) Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684) Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715) fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714) Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713) Fix oppia#3701 Fixed Continue button name issue (oppia#3711) Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650) Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700) Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709) Learner audio updates (oppia#3705) Move hint button to left corner for supplemental cards (oppia#3702) Make the representation of the learner's 'Continue' answer more conversational. (oppia#3675) Add editor for audio translations (oppia#3692) Fix oppia#3686 fixed text overflow for contributors list (oppia#3697) Fix oppia#3551: Manually set cookie value during language change (oppia#3622) Learner view audio translations (oppia#3681) The sixth milestone of the learner dashboard. (oppia#3680) Routine update of translations. (oppia#3685) SiteWide ACL Refactor: Milestone 3.1 (oppia#3682) ...

* upstream/develop: (28 commits) Preloading of audio and bandwidth confirmation (oppia#3727) Fix oppia#3693: Deprecate ClassifierDataModel and update ClassifierTrainingJobModel (oppia#3734) Fix part of oppia#3453: Removed jinja template in editor_navigation_directive (oppia#3732) Eliminates stray tick mark (oppia#3731) Fix oppia#3453: Removed jinja template in collection editor navigation bar directive (oppia#3723) Fix oppia#3726: prevent graph SVG from overlapping modal footer buttons. (oppia#3728) address sean review comments Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717) fix linting remove jinja template in create_activity_modal Fix oppia#2998 implement collection skills update commands (oppia#3710) Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684) Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715) fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714) Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713) Fix oppia#3701 Fixed Continue button name issue (oppia#3711) Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650) Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700) Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709) Learner audio updates (oppia#3705) ...

tjiang11 added 3 commits August 5, 2017 00:13

Add audio not available message

43de573

lint

f7d2137

display audio might not match text message to learner when audio tran…

5cf4018

…slation flagged

tjiang11 requested a review from seanlip August 5, 2017 04:56

tjiang11 changed the title ~~Learner audio updates from #3704~~ Learner audio updates Aug 5, 2017

seanlip requested changes Aug 5, 2017

View reviewed changes

tjiang11 added 3 commits August 5, 2017 10:46

suggested changes

2d2fd01

translate tooltip

ef1969a

remove language code from scope

985c8ab

seanlip approved these changes Aug 5, 2017

View reviewed changes

tjiang11 added 2 commits August 5, 2017 15:16

add param to translation

6dc751f

add to qqq

d52bda0

seanlip added the PR: LGTM label Aug 5, 2017

seanlip merged commit 762d91a into oppia:develop Aug 5, 2017

seanlip mentioned this pull request Aug 6, 2017

Additional updates to the audio functionality #3704

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Learner audio updates #3705

Learner audio updates #3705

tjiang11 commented Aug 5, 2017

seanlip left a comment

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip commented Aug 5, 2017

codecov-io commented Aug 5, 2017 •

edited

Loading

seanlip left a comment

seanlip Aug 5, 2017

tjiang11 Aug 5, 2017

tjiang11 Aug 5, 2017

seanlip Aug 5, 2017

Learner audio updates #3705

Learner audio updates #3705

Conversation

tjiang11 commented Aug 5, 2017

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Aug 5, 2017

codecov-io commented Aug 5, 2017 • edited Loading

Codecov Report

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2017 •

edited

Loading