Fix song playback #6
Conversation
|
Additional features (starting from commit 6057f5e):
NOTE: Request new turntable.glb model file |
frontend/src/App.js
Outdated
| @@ -32,110 +35,21 @@ const Scene = () => { | |||
| const [playing, setPlaying] = useState(false); | |||
| const [soundOn, setSoundOn] = useState(true); | |||
| const [toneArmFinished, setToneArmFinished] = useState(false); | |||
| const coverPicUrls = ["Aiguille.jpg", "CanaryForest.jpg", "Sworn.jpg"]; | |||
| const [coverPicUrl, setCoverPicUrl] = useState("Aiguille.jpg"); | |||
There was a problem hiding this comment.
Maybe do something like this?
const [coverPicUrl, setCoverPicUrl] = useState(coverPicUrls[songIndex])
Also maybe move the songIndex state above this line.
frontend/src/components/Song.js
Outdated
| const y = 1.4; | ||
| const z = 4; | ||
|
|
||
| const [songs, setSongs] = useState(data()); // List of songs |
There was a problem hiding this comment.
If you are not going to use setSongs, then we probably don't need a state. Let me know if it works if you just declare a const and not have it be a state for the component.
There was a problem hiding this comment.
@320834 You're right that we won't need it for now, but I think we might need something similar when we tie in the Spotify API.
IE : If the user builds a queue of songs to listen, then setSongs would be needed to update the list of songs (which would be displayed in the frontend).
Thoughts?
There was a problem hiding this comment.
Yeah understood, but we will add it in when we get to that point.
frontend/src/components/Song.js
Outdated
| const [currentSong, setCurrentSong] = useState({ | ||
| name: "Beaver Creek", | ||
| cover: | ||
| "https://chillhop.com/wp-content/uploads/2020/09/0255e8b8c74c90d4a27c594b3452b2daafae608d-1024x1024.jpg", | ||
| artist: "Aso, Middle School, Aviino", | ||
| audio: "Beaver Creek.mp3", | ||
| color: ["#205950", "#2ab3bf"], | ||
| id: uuidv4(), | ||
| active: true, | ||
| }); |
There was a problem hiding this comment.
Is it possible to set it to the first element of the data?
frontend/src/components/Song.js
Outdated
| useEffect(() => { | ||
| setCurrentSong(songs[0]); | ||
| }, [songs]); |
There was a problem hiding this comment.
I am not sure if this will ever get run. Basically if songs is ever changed, then it will run this effect, but I don't see anything that changes the song state.
https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects
There was a problem hiding this comment.
If the intention of this is to run it upon first render and it does the intended task then disregard this message
There was a problem hiding this comment.
It was indeed intended to run on the first render, especially because the songs list is initialized from a separate function. Sometimes the currentSong is loaded before the songs is loaded in, which crashes the app.
However, I do think the useEffect will need to change - songs is intended to be the Spotify queue of songs for the user. We don't want to set the current song to the first song every time a new song is added to songs (which will happen in the current code).
There was a problem hiding this comment.
I vote let's keep the songs / setSongs parts as is for now, we'll leave it as imperfect code snippets that exist as to-do's once the API is working.
There was a problem hiding this comment.
Okay, just keep a mental note to revisit this later.
frontend/src/components/Turntable.js
Outdated
| useEffect(() => { | ||
| document.body.style.cursor = hovering ? "pointer" : "auto"; // change pointer to finger when hovered | ||
| // document.body.style.cursor = hovering ? "pointer" : "auto"; // change pointer to finger when hovered | ||
| }, [hovering]); |
There was a problem hiding this comment.
If this is not used, then maybe get rid of this useEffect.
…ments to Turntable.js
Several functionalities fixed regarding audio playback / skipping between songs
utils.js, which currently stores the meta data for lofi songsSong.js,Lights.js, andTurntable.js- overall lightening the load onApp.js@320834 Feel free to check out the branch on your local, but I won't merge until we talk about it this weekend.