Add Progress Bar to Song Card (Studio & Marketplace) #617
Conversation
|
Visit the preview URL for this PR (updated for commit 2c38bf7): https://newm-artist-portal--pr617-mrkt-27-single-item-68jalflh.web.app (expires Wed, 26 Jun 2024 06:50:09 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46 |
|
Visit the preview URL for this PR (updated for commit 2c38bf7): https://fan-square--pr617-mrkt-27-single-item-hvp6nakk.web.app (expires Wed, 26 Jun 2024 06:50:57 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b38f4676e459f355dcb11c95730012d43389bfd2 |
|
Visit the preview URL for this PR (updated for commit 697f3ec): |
| // Gives indication that the song is playing by incrementing the progress bar | ||
| if (songProgress === 0) setSongProgress(1); |
There was a problem hiding this comment.
I added this to show progress of the playback from the start of track audio, but no issue removing it too. I think we need to add an extra one second to the duration because of this, but would like to see your and product's opinion.
Standard across other platforms: Would need to remove the line, the first second of progress is typically delayed by the interval loop. Seen on Spotify and others.
| @@ -51,15 +51,15 @@ export const mockSales: ReadonlyArray<Sale> = [ | |||
| costAssetName: "XYZ123", | |||
| costPolicyId: "123XYZ", | |||
| createdAt: new Date("April 10th, 2024").toDateString(), | |||
| id: "3cfb2d02-a320-4385-96d1-1498d8a1df58", | |||
| id: "cec4f704-69c4-41fd-807d-47aa2d73f0d2", | |||
There was a problem hiding this comment.
Added this just for testing, this is one of the backend IDs, so clicking the first track under Just Released leads to a backend example. The First two play icons under Just Released can playback from a nftcdn source that might be similar to what the end url will look like.
Didn't want to change this file up too much since we might be pulling in backend data soon and replacing all of this.
There was a problem hiding this comment.
Noticed a long delay (5 seconds) before the Studio sound card is actually playing audio on initial click. The progress bar is rolling from the initial click... need to figure out how to sync up with the network loading delay. Will create a separate bug ticket for this, probably need to expose the actual audio position which will then help with adding a seek functionality.
There was a problem hiding this comment.
I'd recommend refactoring the code to get the position from the song as part of this ticket, since it would be good for the progress bar to reflect the position of the song for the MVP. The seek functionality doesn't need to be in the MVP though.
There was a problem hiding this comment.
The Sales component and Sale page are separated so the playback of the sound card doesn't know the other instance exists. Aka you can play the Sale page asset and one of the More of Artist SongCards at the same time.
Will create another ticket/branch to include Redux or universal playback tracking across marketplace.
There was a problem hiding this comment.
Sounds good. It may have already been your intention, but I'd also incorporate the Redux functionality into the hooks so that it's abstracted there.
|
Visit the marketplace preview URL for this PR (updated for commit edf6b24): |
|
Visit the wallet preview URL for this PR (updated for commit edf6b24): |
|
Visit the marketplace preview URL for this PR (updated for commit ae6b1f0): |
|
Visit the wallet preview URL for this PR (updated for commit ae6b1f0): |
|
Visit the wallet preview URL for this PR (updated for commit 809d60d): |
|
Visit the marketplace preview URL for this PR (updated for commit 809d60d): |
|
Visit the tools preview URL for this PR (updated for commit 809d60d): |
|
Visit the tools preview URL for this PR (updated for commit 8d57b4e): |
|
Visit the marketplace preview URL for this PR (updated for commit 8d57b4e): |
| @@ -71,7 +71,7 @@ export const extendedApi = newmApi.injectEndpoints({ | |||
| ...(statuses ? { statuses: statuses.join(",") } : {}), | |||
| ...rest, | |||
| }, | |||
| url: "/v1/marketplace/sales", | |||
| url: "v1/marketplace/sales", | |||
There was a problem hiding this comment.
good catch, i am bad with these 😅
packages/utils/src/lib/hooks.ts
Outdated
| videoRef.current.addEventListener("ended", handleSongEnded); | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This works, but in order to not have to ignore the line, it should be possible to create all dependent functions using useCallback, if the concern is that including the functions as dependencies will cause the function to be called more often than it should. I didn't test it, so I imagine the comment may be necessary if that approach still results in the function being called at the incorrect time, but wanted to mention it in case.
Also, if it is necessary, it can be helpful to add another comment above the eslint-disable-next-line comment, just to clarify why including all dependencies doesn't work.
There was a problem hiding this comment.
Added the callbacks, and added the appropriate eslint-disable with the functions, playSongWithHlsJs and playSongNatively, that would have caused issues if dependencies of trackSongProgress were included.
|
Visit the wallet preview URL for this PR (updated for commit 8d57b4e): |
|
Visit the wallet preview URL for this PR (updated for commit 1532d03): |
|
Visit the tools preview URL for this PR (updated for commit 1532d03): |
|
Visit the marketplace preview URL for this PR (updated for commit 1532d03): |
dmkirshon
deleted the
MRKT-27-Single-Item-for-Play_Add-Progress-Bar-to-Song-Card
branch
Add:
Note: Change affects all sound cards
demo_Marketplace_Song-Card-Progress-Bar.mov
demo_Studio_Song-Card-Progress-Bar.mov