Revamped, Curved Library Layout, Better Player Initialization, and Clean Animations #180
Conversation
Still not there yet.
- Add semi-transparent navigation arrows to Library scroll wheels with hover effects - Reduce section spacing (35vh → 15vh) and title margins (8vh → 4vh) - Implement persistent shimmer effect for currently playing tracks across page navigation - Fix device initialization with better state management and error handling - Add exponential backoff for rate-limited API calls - Improve player state persistence and device activation logic - Add proper cleanup for player disconnection - Enhance error handling for 404, 429, and 500 responses - Add initialization state tracking to prevent race conditions
-Add smooth fade animations for song title, artist name, and album artwork in Home.js -Implement matching gradient footer animations in Library.js for consistent transitions -Fix song title transition flickering by adjusting AnimatePresence behavior -Reduce initial spacing in Library view by decreasing top padding from 10vh to 5vh -Standardize animation timings across components for a cohesive experience -Maintain responsiveness of Home.js while improving visual transitions
tennitech
added
enhancement
EvWhymark
left a comment
EvWhymark
left a comment
There was a problem hiding this comment.
There is far too many things being tried here. This could have been like 10 PRs and I still would have said you can split it up more. Why are we writing a 1400 line change PR? Furthermore, a lot of this code makes no sense. Functionally it barely works. It looks good but as a feature it is questionable. I understand you want to merge this for the Expo but this creates too many issues that we would have to resolve. I can not approve this.
For the Slider I think if you wanted to implement this you should have just done the arrow buttons. Those do work as they are now and that should have been the end of this PR. Adding scalability and dragging handling I think was too much and thus those two features suffered.
For the Player I still do not know what the issue is. Why was 338 line changes done on a file without properly addressing what is going on?
For Home.js the animations are a good idea. The album art I think was even well executed. But the dragging functionality for recently played is broken. And I mentioned this in the review but the animated text does not seem like it is working.
| <AnimatePresence mode="wait"> | ||
| <motion.h1 | ||
| key={track?.name || "Unknown"} | ||
| initial={{ opacity: 0, y: 20 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| exit={{ opacity: 0, y: -20 }} | ||
| transition={{ duration: 0.5, ease: "easeOut" }} | ||
| style={{ | ||
| fontFamily: 'Loubag, sans-serif', | ||
| fontSize: 'clamp(2.2rem, 4vw, 4rem)', | ||
| margin: '0', | ||
| textAlign: 'left', | ||
| color: '#ECE0C4', | ||
| textShadow: ` | ||
| 2px 2px 0 rgba(255,0,0,0.2), | ||
| -2px -2px 0 rgba(0,0,255,0.2), | ||
| 1px -1px 0 rgba(255,0,255,0.2) | ||
| `, | ||
| animation: 'textGlitch 3s infinite' | ||
| }} | ||
| > | ||
| {track?.name || "Unknown"} | ||
| </motion.h1> | ||
| </AnimatePresence> | ||
|
|
||
| <AnimatePresence mode="wait"> | ||
| <motion.h2 | ||
| key={track?.artists?.map(artist => artist.name)?.join(", ") || "Unknown"} | ||
| initial={{ opacity: 0, y: 20 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| exit={{ opacity: 0, y: -20 }} | ||
| transition={{ duration: 0.5, ease: "easeOut", delay: 0.1 }} | ||
| style={{ | ||
| fontFamily: 'Notable, sans-serif', | ||
| fontSize: 'clamp(1.2rem, 2vw, 1.6rem)', | ||
| margin: '0', | ||
| opacity: 0.9, | ||
| letterSpacing: '1px', | ||
| color: 'white' | ||
| }} | ||
| > | ||
| {track?.artists?.map(artist => artist.name)?.join(", ") || "Unknown"} | ||
| </motion.h2> | ||
| </AnimatePresence> |
There was a problem hiding this comment.
Dont think this animation is working the way as it should. Is this the glitchy one you mentioned?
| import cloudsSvg from '../assets/clouds.svg'; | ||
| import playIcon from '../assets/play-icon.svg'; | ||
| import pauseIcon from '../assets/pause-icon.svg'; | ||
| import ColorThief from "color-thief-browser"; |
There was a problem hiding this comment.
These changes should have been put in a different PR
| <style> | ||
| {` | ||
| @keyframes progressShimmer { | ||
| 0% { | ||
| background-position: -200% center; | ||
| } | ||
| 100% { | ||
| background-position: 200% center; | ||
| } | ||
| } | ||
| .progress-bar { | ||
| position: relative; | ||
| width: 100%; | ||
| height: 8px; | ||
| background-color: rgba(236, 224, 196, 0.2); | ||
| border-radius: 4px; | ||
| cursor: pointer; | ||
| overflow: hidden; | ||
| } | ||
| .progress-indicator { | ||
| position: absolute; | ||
| height: 100%; | ||
| background-color: #ECE0C4; | ||
| border-radius: 4px; | ||
| transition: width 0.1s linear; | ||
| overflow: hidden; | ||
| } | ||
| .progress-indicator::before { | ||
| content: ''; | ||
| position: absolute; | ||
| top: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| left: 0; | ||
| background: linear-gradient( | ||
| 90deg, | ||
| transparent, | ||
| rgba(255, 255, 255, 0.2), | ||
| transparent | ||
| ); | ||
| background-size: 200% 100%; | ||
| animation: progressShimmer 2s infinite; | ||
| pointer-events: none; | ||
| z-index: 1; | ||
| } | ||
| `} | ||
| </style> |
There was a problem hiding this comment.
Put this in a css file. Like app.css
| import { PlayerContext } from './Player'; | ||
| import '../App.css'; | ||
| import backgroundPng from '../assets/background.png'; | ||
| import defaultAlbumArt from '../assets/default-art-placeholder.svg'; |
There was a problem hiding this comment.
Should have been put into a new component file. 834 line additions is crazy. This file is now larger than Home.js
| const RESPONSIVE_ITEM_SIZE = 'clamp(100px, 10vw, 150px)'; // Item size between 100px and 150px | ||
| const RESPONSIVE_VIEW_HEIGHT = 'clamp(280px, 30vh, 400px)'; // View height between 280px and 400px | ||
| const RESPONSIVE_SPACING = 'clamp(130px, 12vw, 180px)'; // Spacing between 130px and 180px | ||
| const RESPONSIVE_ARC_HEIGHT = 'clamp(30px, 5vh, 60px)'; // Arc height between 30px and 60px | ||
| const RESPONSIVE_TITLE_SIZE = 'clamp(1.6rem, 3vw, 2.5rem)'; | ||
| const RESPONSIVE_TITLE_SPACING = 'clamp(2px, 0.5vw, 4px)'; |
There was a problem hiding this comment.
Why not just put these in line like everything else?
| // Only retry on rate limiting or server errors | ||
| if (error.response?.status === 429 || error.response?.status === 500) { | ||
| setActivationAttempts(prev => prev + 1); | ||
| // Exponential backoff for retries | ||
| retryTimeout = setTimeout(() => { | ||
| if (isMounted) { | ||
| activateDevice(); | ||
| } | ||
| }, Math.min(1000 * Math.pow(2, activationAttempts), 8000)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is rate limiting really a concern? How would I test this?
| activateDevice(); | ||
|
|
||
| return () => { | ||
| player.removeListener("player_state_changed", syncTrack); |
There was a problem hiding this comment.
I do agree the current Player.js does not properly deallocate the listener. This would make a great issue. This should not have been done here and I am not fully convinced it is being done here in this current iteration.
| }, [player]); | ||
| }, [deviceId, accessToken, playerReady, invalidateAccess, currentPlaybackState, activationAttempts, isInitializing]); | ||
|
|
||
| //TODO: Do better error catching |
There was a problem hiding this comment.
Why remove this. I do not think this was accomplished.
| deviceReady: deviceActivated && playerReady, | ||
| playingUri |
There was a problem hiding this comment.
What is the difference between playingUri and playUri? Also where are deviceReady being used. While these conditionals make sense to solve the race condition that I mentioned in a previous PR this doesn't seem like the right solution. That should be handled upstream where the data is initialized not by some conditional that will create overhead.
| p.addListener("ready", ({ device_id }) => { | ||
| setDeviceId(device_id); | ||
| const newPlayer = new window.Spotify.Player({ | ||
| name: 'JukeOS Player', |
There was a problem hiding this comment.
We are not JukeOS. I called it Juke Spotify Player before
|
Also please resolve your merge conflicts. |
What’s this about?
This PR polishes the playback experience, tightens Library spacing, and hardens player connectivity. It introduces smooth cross‑fade animations for track changes, adds clearer navigation cues, and eliminates several edge‑case crashes and API‑rate‑limit failures.
What’s changed?
UI / UX
Home.js
Library.js
Player logic
Result
Testing
Closes issue #33.