HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

A simple MP3 player with React.js

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplewithplayerreactmp3

Problem

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:

-
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?

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

ReactDOM.render(
       ,
       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:

```

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

},
render: function () {

return (
{this.props.soundTitle}
{this.props.soundLength}

);
}
});

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 retu

Solution

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(
    ,
    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(
            
                
                
                
              
        );  
    }
})


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 (
        
            
            {props.sounds.map(function(sound) {
                return (
                    
                      {sound.soundTitle}
                      {sound.soundLength}
                    
                );
            })}
            
         
    );
}


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

Code Snippets

var sounds = [...];

ReactDOM.render(
    <MusicPlayer sounds={sounds} />,
    document.getElementById('app')
);
var MusicPlayer = React.createClass({            
    componentDidMount: function() {
        this.player = document.getElementById('audio_player');
    }
    ...
})
var MusicPlayer = React.createClass({            
    ...
    render: function () {       
        return(
            <div id="music_player">
                <Sounds />
                <Controls />
                <AudioPlayer />
            </div>  
        );  
    }
})
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> 
    );
}

Context

StackExchange Code Review Q#162824, answer score: 5

Revisions (0)

No revisions yet.