patternjavascriptMinor
A simple MP3 player with React.js
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
-
As you can see towards the end of the code I have this:
I was wondering is there a way to detect the HTML has finished being "rendered" so that I can use JavaScript
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
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:
Second question:
For this I would look at using a lifecyle hook [1], specifically
So, for example:
As a side note: I know too little about the
Just some general observations/suggestions:
I would look to make the
So, in the end the
Also, as you can tell above, I would convert the
Something like:
Additionally, I would look to simplify the way you set the next and previous sounds as the current sound, by simply setting the
[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
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.