Feature/media volume for static media #1118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gfodor
commented
Mar 27, 2019
| componentData.controls || isHubsDestinationUrl(componentData.src) || isHubsDestinationUrl(componentData.href); | ||
|
|
||
| const hasVolume = componentName === "video"; |
netpro2k
reviewed
Apr 3, 2019
| @@ -131,13 +131,18 @@ AFRAME.GLTFModelPlus.registerComponent("media", "media", (el, componentName, com | |||
| }); | |||
|
|
|||
| function mediaInflator(el, componentName, componentData, components) { | |||
| let isControlled = true; | |||
There was a problem hiding this comment.
Seems like you are only using this within the conditional, why define it as a let out here instead of a const inside like before?
There was a problem hiding this comment.
its used later on line 173
| class="ui video-seek-forward-button" | ||
| visible="false" | ||
| ></a-image> | ||
| <a-entity class="video-playback"> |
There was a problem hiding this comment.
Oof I hate to add another level here just for this. You already have a ref to back/forward/play.. Seems ok to just hide them directly.
This was referenced Nov 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the behavior of Spoke-embedded media so that videos will have volume controls -- it seems right that we should always allow users to adjust the volume of videos in the room even if they can't control them otherwise.
This PR also removes the visual effects on hovering of controllable static media -- on balance this visual effect is likely not necessary and would be distracting for large, fixed videos etc where only the volume controls are accessible.