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

Modal audio #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Modal audio #29

wants to merge 3 commits into from

Conversation

JeffTomlinson
Copy link

@JeffTomlinson JeffTomlinson commented Jul 29, 2017

Modal audio

This PR introduces the following changes:

  • Adds a sound option to modals
  • Note: This has already been tested and implemented in the Meredith VR project.

Steps to Test

  • Checkout this branch.
  • Run yarn or npm i in this repo's root.
  • Run npm run start in this repo's root.
  • Navigate to the IP address:port given to you in the start script output in Chrome on your phone, which should be connected to the same network as your computer. (And not using a VPN).
  • Add an mp3 asset.
  • Add the following properties to a modal entity:
sound={require('../../assets/sounds/your-sound.mp3')}
volume="5"
  • Verify that the sound plays when opening the modal and stops when closing.

Copy link
Contributor

@patrickocoffeyo patrickocoffeyo left a comment

Choose a reason for hiding this comment

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

Couple small comments :)

@@ -195,6 +248,8 @@ Modal.defaultProps = {
title: 'Please give me a title :)',
content: 'Please give me some content :)',
image: '',
sound: '',
volume: '10',
Copy link
Contributor

Choose a reason for hiding this comment

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

This can likely be a number :)

@@ -195,6 +248,8 @@ Modal.defaultProps = {
title: 'Please give me a title :)',
content: 'Please give me some content :)',
image: '',
sound: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this exists conditionally, I'd make this a boolean, and instead of having a hasSound() method, just check if (this.props.sound) { // do stuff }

Copy link
Author

@JeffTomlinson JeffTomlinson Aug 4, 2017

Choose a reason for hiding this comment

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

@patrickocoffeyo so in the case that this has a sound, the value for this property would be the sound file (i.e. a string). For example it would be defined in the modal like so:

<Modal
    id="modal__upstairs"
    title="Upstairs Playroom"
    position={{ x: -26.15, y: 5.70, z: -10.41 }}
    image={require('../../assets/images/jpg/upstairs.jpg')}
    sound={require('../../assets/sounds/upstairs.mp3')}
    volume={5}
 />

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that makes sense, and if no sound property is specified, it'll default to the value you set here. If that value is just false, it'll make checking for the existence of the sound a bit easier :) I should probably do that for image as well. I think that will work..

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nope, I'm wrong. You can't default a string to a boolean, it does do typing on defaults. I coulda sworn that wasn't true! Oh well. Sorry about that!

* @returns {boolean}
* Returns true if a sound is set, otherwise returns false.
*/
hasSound() {
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment below about defaulting this to false. II think it should work if we type check this to string, and default it to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was totes wrong on this, you defaults are type checked. 🛑 This is good to go!

@JeffTomlinson
Copy link
Author

JeffTomlinson commented Aug 4, 2017

@patrickocoffeyo I made the change to the volume propType. I had actually done this in a more recent version of this for Meredith. Incidentally some more recent work on that project also breaks out the sound playback methods into a separate class that can be shared across multiple components (like the existing Sound component). I've been meaning to PR that work here as well. Up to you if you think this PR is useful as is or if you want to wait for the additional enhancements.

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.

2 participants