6
\$\begingroup\$

I've been learning React.js for the last few days and am working on a simple MP3 player. I have a few years experience with JavaScript, however, I am just trying to get used to the idea of components in React.js. I would also like to be following best practices as I may use the application/code as a portfolio piece.

What I have made is working correctly. However, I was wondering if you can look through my code and see if I could do anything better/more efficiently.

Also, I have the following specific questions:

  1. In the MusicPlayer component I have an array of objects with information on different sounds. I am aware I have this "sounds" array set up as a "state" and I was wondering is there another way to incorporate this array perhaps as a property (seeing as it won't be changing state throughout the application). Or do you think it is ok, the way I have done it?

  2. As you can see towards the end of the code I have this:

    ReactDOM.render(
       <MusicPlayer />,
       document.getElementById('app')
);

    I was wondering is there a way to detect the HTML has finished being "rendered" so that I can use JavaScript document.getElementById on one of the elements. At the moment I am using window.onload for this.

Full code:

<div id="app"></div>

<script type="text/babel">

    var Sound = React.createClass({
        //make a component for individual sounds
        //these will be html <li> list items
        getInitialState: function () {
            return {isSelected: false}

        },
        render: function () {

            return (<li className="sound_list_item">
                    <span className="sound_title">{this.props.soundTitle}</span>
                    <span className="sound_length">{this.props.soundLength}</span>
                </li>
            );
        }
    });

    var MusicPlayer = React.createClass({
        //this will be our main component i.e a parent to the Sound component and also the Controls component (defined below)
        getInitialState: function () {
            //return an object with an array of all of our sounds. The sounds array itself will not change state.
            //also return the index of the current sound. This will start at 0 and change on pressing previous and next.
            return {
                sounds: [{"title" : "Falling Cute", "length": "0:03", "mp3" : "sounds/0027_falling_cute.mp3"}, 
                    {"title" : "Feather", "length": "0:06", "mp3" : "sounds/0028_feather.mp3"},
                    {"title" : "Lose Cute", "length": "0:03", "mp3" : "sounds/0036_lose_cute.mp3"}, 
                    {"title" : "Pium", "length": "0:03", "mp3" : "sounds/0039_pium.mp3"}],
                currentSoundIndex: 0
            }
        },
        getSoundInfo: function (soundIndex, soundsArray){
            //this function takes in the index of the sound and returns information on that sound.
            //var sounds = this.state.sounds; 
            var songTitle = soundsArray[soundIndex].title;
            var songMp3 = soundsArray[soundIndex].mp3;
            var soundInfo = {"title": songTitle, "mp3": songMp3};
            return soundInfo;   
        },
        getCurrentSoundInfo: function (){
            //this function returns information on the current sound.
            var sounds = this.state.sounds; //make a copy of the state of the sounds
            var currentIndex = this.state.currentSoundIndex;
            var currentSoundInfo = this.getSoundInfo(currentIndex, sounds);
            return currentSoundInfo;    
        },
        getPreviousSoundInfo: function (){
            //this function returns information on the previous sound in the array.
            //it also sets the currentIndex to be the previous index
            var sounds = this.state.sounds; //make a copy of the state of the sounds
            var currentIndex = this.state.currentSoundIndex;

            //get the index of the previous sound in the array
            var previousIndex = currentIndex - 1;

            //set the new currentSoundIndex state to be the previous index
            this.setState({currentSoundIndex: previousIndex}); 

            //return the info on this sound
            var previousSoundInfo = this.getSoundInfo(previousIndex, sounds);
            return previousSoundInfo;
        },
        getNextSoundInfo: function (){
            //this function returns information on the next sound in the array.
            //it also sets the currentIndex to be the next index
            var sounds = this.state.sounds; //make a copy of the state of the sounds
            var currentIndex = this.state.currentSoundIndex;

            var nextIndex = currentIndex + 1;

            //set the new currentSoundIndex state to be the next index
            this.setState({currentSoundIndex: nextIndex});

            //return the info on this sound
            var nextSoundInfo = this.getSoundInfo(nextIndex, sounds);
            return nextSoundInfo;   
        },
        eachSound: function (sound, i) {
            return (<Sound key={i} index={i} soundTitle={sound.title} soundLength={sound.length} >
                </Sound>
            );

        },
        render: function () {       
            return(<div id="music_player">
                    <div className="scrollable_container scrollable">
                        <ul id="list_of_sounds">
                        {
                        //call the eachSound method on each item of the sounds array
                        this.state.sounds.map(this.eachSound)
                        }
                        </ul>
                    </div>

                    <Controls getCurrentSound={this.getCurrentSoundInfo} getPreviousSound={this.getPreviousSoundInfo} getNextSound={this.getNextSoundInfo} />
                </div>  
            );  
        }
    });

    var Controls = React.createClass({
        //make a component for the controls of the audio player. Simple for the moment: previous, play and next.
        getInitialState: function () {
            //we will need a state which tells us the url of the src of our audio tag.
            //use the getCurrentSound() property to get the url of the current mp3.
            var currentSoundInfo = this.props.getCurrentSound();

            return {mp3Src: currentSoundInfo.mp3} 

        },
        startPlayer: function (){
            //load the html5 player and play the sound
            player.load();
            player.play();
        },
        playSound: function (){

            var currentSoundInfo = this.props.getCurrentSound();

            //set the mp3Src state to be the mp3 url of the sound that we need to play now
            this.setState({mp3Src: currentSoundInfo.mp3});

            //now that we've set the src attribute of the HTML5 audio element we call startPlayer 
            this.startPlayer();
        },
        goToPreviousSound: function (){
            var previousSoundInfo = this.props.getPreviousSound();

            //set the mp3Src state to be the mp3 url of the sound that we need to play now
            this.setState({mp3Src: previousSoundInfo.mp3});
            console.log(previousSoundInfo.mp3);
            //now that we've set the src attribute of the HTML5 audio element we call startPlayer 
            this.startPlayer();
        },
        goToNextSound: function (){
            var nextSoundInfo = this.props.getNextSound();

            //set the mp3Src state to be the mp3 url of the sound that we need to play now
            this.setState({mp3Src: nextSoundInfo.mp3});
            console.log(nextSoundInfo.mp3);

            //now that we've set the src attribute of the HTML5 audio element we call startPlayer 
            this.startPlayer();
        },
        render: function () {

            return (<div id="controls_area">

                    <div id="controls">

                        <button onClick={this.goToPreviousSound} className="btn_control"><i className="fa fa-backward"></i></button>
                        <button onClick={this.playSound} className="btn_control"><i className="fa fa-play"></i></button>
                        <button onClick={this.goToNextSound} className="btn_control"><i className="fa fa-forward"></i></button>
                    </div>
                    <audio id="audio_player">
                        <source id="src_mp3" type="audio/mp3" src={this.state.mp3Src}/>
                        <source id="src_ogg" type="audio/ogg" src=""/>
                        <object id="audio_object" type="audio/x-mpeg" width="200px" height="45px" data={this.state.mp3Src}>
                            <param id="param_src" name="src" value={this.state.mp3Src} />
                            <param id="param_src" name="src" value={this.state.mp3Src} />
                            <param name="autoplay" value="false" />
                            <param name="autostart" value="false" />
                        </object>
                    </audio>

                </div>
            );  
        }
    });


    //take MusicPlayer HTML and display it in div with id app
    ReactDOM.render(
    <MusicPlayer />,
    document.getElementById('app')
    );

    </script>



    <script>
    var player;
    window.onload = function(){
        //now that the window has loaded and our html should be rendered, create a javascript player object from our HTML5 audio element.
        //we will have only one audio element on the page even though we have several sounds that can be played.
        //in order to play different sounds we will reset the "src" attribute of the audio player with the new url of the selected sound.
        player = document.getElementById('audio_player');

    }
    </script>
\$\endgroup\$
1
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ Commented May 9, 2017 at 18:26

1 Answer 1

Reset to default
5
\$\begingroup\$

First off, well done on getting it to work - that's often the most important and anything beyond that comes with experience and following the docs and guidelines.

First question:

In my experience your intuition to use a prop instead of storing the sounds in state is the right one. It is essentially ok the way you have it in state, but the less state your component stores and has to keep track of the easier it will be to reason about your code and track down any future bugs.

So something like:

var sounds = [...];

ReactDOM.render(
    <MusicPlayer sounds={sounds} />,
    document.getElementById('app')
);

Second question:

For this I would look at using a lifecyle hook [1], specifically componentDidMount() in your MusicPlayer class.

So, for example:

var MusicPlayer = React.createClass({            
    componentDidMount: function() {
        this.player = document.getElementById('audio_player');
    }
    ...
})

As a side note: I know too little about the player to actually comment on it's load() and play() functions. If you want that reviewed I'd need more info.

Just some general observations/suggestions:

I would look to make the Sound and Controls components stateless functional components [2]. As well as create an own stateless component for AudioPlayer that just takes a single prop of mp3src. And, in the process of doing that, move all state up and into the MusicPlayer to have that as your single point of truth (which, again, as mentioned above will make it easier to reason about your code and track down any bugs that may occur).

So, in the end the MusicPlayer's render function would resemble something like:

var MusicPlayer = React.createClass({            
    ...
    render: function () {       
        return(
            <div id="music_player">
                <Sounds />
                <Controls />
                <AudioPlayer />
            </div>  
        );  
    }
})

Also, as you can tell above, I would convert the Sound into more of a Sounds list component and move the DOM elements for it out of the MusicPlayer, meaning that the MusicPlayer need not be concerned with it's content.

Something like:

var Sounds = function(props) {
    return (
        <div className="scrollable_container scrollable">
            <ul id="list_of_sounds">
            {props.sounds.map(function(sound) {
                return (
                    <li className="sound_list_item">
                      <span className="sound_title">{sound.soundTitle}</span>
                      <span className="sound_length">{sound.soundLength}</span>
                    </li>
                );
            })}
            </ul>
        </div> 
    );
}

Additionally, I would look to simplify the way you set the next and previous sounds as the current sound, by simply setting the currentSoundIndex and using that to determine the mp3src prop that is handed down to the stateless components that require it in the MusicPlayer's render function. Using that you can reduce the complexity of the class functions and remove the need for a getSoundInfo function.

[1] https://facebook.github.io/react/docs/state-and-lifecycle.html

[2] https://facebook.github.io/react/docs/components-and-props.html#functional-and-class-components

\$\endgroup\$
7
  • \$\begingroup\$ thanks for all your advice. Your answer really helps, especially passing sounds in as a property. :) \$\endgroup\$ Commented May 9, 2017 at 12:05
  • \$\begingroup\$ You're welcome. My first answer on here so I hope it was all clear and understandable. \$\endgroup\$ Commented May 9, 2017 at 12:36
  • \$\begingroup\$ yes it was very clear :) I'm working on some of your suggestions. Making "sounds" a property (instead of a state) is working well. Your idea to make Sounds a stateless component is a good idea too. However I remember the reason why i made a stateful Sound component and it was because, on developing the player further I would like to know which sound is currently playing so that I can add a CSS className to the song currently playing in the list. And so I had the idea to have a state for each sound as you can see in my code {isSelected: false}.I will try out the different ideas anyway thanks \$\endgroup\$ Commented May 9, 2017 at 12:53
  • 1
    \$\begingroup\$ Ah I see, you could pass in the currentSoundIndex as an additional prop to the stateless component and then on mapping over the sounds list if the index is the current one set the CSS class on that li element (keeping it all stateless and saving you from tracking more state) \$\endgroup\$ Commented May 9, 2017 at 13:02
  • 1
    \$\begingroup\$ Ah good spot. Edited 😊 \$\endgroup\$ Commented May 9, 2017 at 14:45

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.