Adding MuteMusic UI #45632
Adding MuteMusic UI #45632
Conversation
hannahbergam
changed the title
…ntuser redux with mutemusic param
| config.skin.staticAvatar = MEDIA_URL + 'Sliced_Parts/Agent_Neutral.png'; | ||
| config.skin.smallStaticAvatar = | ||
| MEDIA_URL + 'Sliced_Parts/Agent_Neutral.png'; | ||
| config.skin.failureAvatar = MEDIA_URL + 'Sliced_Parts/Agent_Fail.png'; | ||
| config.skin.winAvatar = MEDIA_URL + 'Sliced_Parts/Agent_Success.png'; | ||
| config.level.levelTracks = levelTracks; |
There was a problem hiding this comment.
Sorry, I'm still having a little trouble putting all the pieces together. Why do we need to set this here?
There was a problem hiding this comment.
If we just need to know if there is background music, maybe set config.level.hasBackgroundMusic instead?
| @@ -31,13 +32,18 @@ export const setInitialData = serverUser => ({ | |||
| type: SET_INITIAL_DATA, | |||
| serverUser | |||
| }); | |||
| export const setMuteMusic = isBackgroundMusicMuted => ({ | |||
There was a problem hiding this comment.
I'm curious to hear Maureen's opinion, but I was thinking the current_user api would only provide very limited profile information (but I see we've also since added hasSeenStandardsReportInfo at some point).
I see that we're already sending this information down in user_app_options (which is kind of where I expected it). Why do we also need it here?
There was a problem hiding this comment.
If it's already being passed from the backend in another place, let's set that value to redux instead of setting it from the API response.
There was a problem hiding this comment.
@maureensturgeon Do we want to use redux here because that's the pattern in TopInstructions? If it were possible to get the needed data to this component without redux, would that be better or worse?
There was a problem hiding this comment.
I think what we're trying to avoid by using redux is the prop drilling, there's a lot of room for error with prop drilling and it also is harder to follow
There was a problem hiding this comment.
If ya'll want to explore what it would take to pass the component data without redux, I definitely think it's worth exploring that though!
| @@ -379,7 +379,7 @@ def app_options | |||
| @app_options[:experiments] = | |||
| Experiment.get_all_enabled(user: current_user, section: section, script: @script).pluck(:name) | |||
| @app_options[:usingTextModePref] = !!current_user.using_text_mode | |||
| @app_options[:muteMusic] = !!current_user.mute_music | |||
| @app_options[:muteMusic] = current_user.mute_music? | |||
There was a problem hiding this comment.
You may also need to send this down in ApiController#user_app_options somewhere. (Sorry, this code isn't very well factored at the moment.)
…k changes are finalized
…in charge of all mute activities directly
…code-dot-org into hbergam/mute-music
This PR adds a music mute button to starwars and minecraft levels. Other levels are unchanged: only leveltypes with levelTracks loaded into their MusicControllers (i.e. levels with background music separate from the other sounds of the lab) will receive this button. BackgroundMusicMuteButton is a functional component- my first!
This is a long PR, but nearly half of the additions are the creation of the BackgroundMusicMuteButton component. The infrastructure to set up the mute functionality in craft and starwars lie in the craft.js and studio.js files. Though most of this code is repeated across files, this follows the existing patterns: there is no single abstracted craft.js file. If there were, much of the existing lines could be deduped. There are some changes made to the test files as well.
The setting is saved in UserPreferences, meaning the toggle will persist for a given user across levels. To account for signed out users, this preference is also saved as a cookie on the browser. If either of these settings returns 'true', the music will be muted. Upon toggling back to the 'on' state, the cookie is removed.
When a user clicks background music back on, the track plays from the beginning. When the user presses 'mute', the track stops immediately.
There is a firehose event to track this behavior, which records whether the user chose to mute/unmute, and which lab the event occurred on (starwars vs minecraft).
The design of the data flow is atypical and based on the fact that TopInstructions receives all its data from redux. I pass mute and unmute functions through redux that are then called from the component directly.
Images of "On" state:
The icon for "Off" has to differ from the spec because we don't have the latest FontAwesome set of icons. Thus, the off button has a volume-off icon instead of the desired 'music-slash'.
Links
Testing story
I updated the relevant tests in InstructionsTest.js, and added testing in InstructionsHeaderTest.
Deployment strategy
Follow-up work
When FontAwesome is updated, switch the 'off' state icon from 'volume-off' to 'music-slash'. (https://codedotorg.atlassian.net/browse/LP-2323)
Privacy
Security
Caching
PR Checklist: