This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Share audio loaded state through pinia #1529
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1529 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
🕹 aspect: interface
|
Size Change: +258 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
obulat
reviewed
Jun 16, 2022
There was a problem hiding this comment.
Great catch!
I was also thinking that we could update the duration in the Pinia store as soon as it is updated from the provider. This would make the audio replays more consistent.
I wonder if it makes more sense to make the Audio list items editable, and add the isLoaded property there? This way we could avoid creating a new Pinia store. I do have reservation about making the audio item not a const, as that might cause other problems if it is updated incorrectly, but it seems like a more flexible approach if we need to update some properties after the list was loaded from the API.
The same approach could be used to update the filetype of the images, otherwise if we open an image without a filetype twice, we would make 2 head requests.
|
That's a good idea, Olga. I will try to implement that here and see if it's viable for a future issue to do with the duration (and probably status too) as you suggested. |
sarayourfriend
force-pushed
the
fix/audio-loading-indicator-page-nav
branch
from
bc9a3c2 to
63cd651
Compare
obulat
reviewed
Jun 17, 2022
obulat
reviewed
Jun 17, 2022
There was a problem hiding this comment.
Any media item can be stored in three stores:
- the search results store (
media/index.ts) - the related items store (
media/related-media.ts) - the single result store (
media/single-result.ts)
When getting an item by id in the media store(/media/index.ts), for instance, we try both the search results and the related media (I'm not sure why we don't look in the single-result 😬):
getItemById: (state) => {
return (mediaType: SupportedMediaType, id: string): Media | undefined => {
const itemFromSearchResults = state.results[mediaType].items[id]
if (itemFromSearchResults) return itemFromSearchResults
return useRelatedMediaStore().getItemById(id)
}
},
|
Another bug I've noticed is if you click on play for some audio in the 'All results` view, and then click on a different audio link to open the single result, the first audio keeps playing, and there's no way to stop it. Edit: this happens on main, too. It's good for mobile where there's the Global audio player, but not for desktop. |
sarayourfriend
force-pushed
the
fix/audio-loading-indicator-page-nav
branch
from
7208682 to
0555098
Compare
obulat
reviewed
Jun 23, 2022
| ) { | ||
| const item = this.getItemById(type, id) | ||
| if (item) { | ||
| Object.assign(item, properties) |
There was a problem hiding this comment.
I didn't realize that you could update the store objects like this!
There was a problem hiding this comment.
I didn't either! I was pleasantly surprised that it worked. But it makes sense, I suppose, as Object.assign mutates the existing object which is a Vue reactive (I think?) or at least part of a deep-reactive tree...
There was a problem hiding this comment.
The fact that it works is even more bizarre because the Vue docs state that properties added using Object.assign will not be reactive. Maybe that's changed in the composition API 🤷?
There was a problem hiding this comment.
Strange. I'm doing something wrong in this code sandbox because nothing is working as expected, but when I try Object.assign(obj, { [count]: count ]) in the doMutation function it doesn't cause the computed to update. When I do obj[count] = count however, it also doesn't cause the computed to update...
https://codesandbox.io/s/relaxed-black-nqhos7?file=/src/components/HelloWorld.vue
Any ideas? I could definitely see Object.assign bypassing the Proxy that Vue uses to track reactive objects...
There was a problem hiding this comment.
Okay, I realized that what I was doing in that previous example was slightly different. I've updated it with something more representative (watching a specific property of the obj) and it works.
Maybe the composition API changed this after all.
obulat
approved these changes
Jun 23, 2022
dhruvkb
approved these changes
Jun 24, 2022
| ) { | ||
| const item = this.getItemById(type, id) | ||
| if (item) { | ||
| Object.assign(item, properties) |
There was a problem hiding this comment.
The fact that it works is even more bizarre because the Vue docs state that properties added using Object.assign will not be reactive. Maybe that's changed in the composition API 🤷?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1528 by @zackkrida
Description
Based on my debugging it seemed like the issue was that the
loadedevent was never firing for the audio after it had already been loaded on the search results page, so when the audio track component is initialized on the single result page,hasLoadedis set to false and never updated totruebecause theloadedevent doesn't fire again after the first time. This made the status get stuck onloading.This PR now follows @obulat's suggestion to manage the audio state within the audio object itself so that it's easily shared throughout the app without the need for an additional store. This treats the audio object itself as the source of truth. It works great I think! And is maybe a little less unexpected. Thanks for the suggestion @obulat!
Testing Instructions
Follow the repro instructions in the linked issue and confirm it's no longer reproducible.
I'm not sure whether to add an e2e test for this as I don't know whether we can reliably mock audio responses at the moment? It's probably worth looking into though.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin