Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: videos in collection are not loading #8729

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Conversation

hassnian
Copy link
Contributor

@hassnian hassnian commented Dec 22, 2023

PR Type

  • Bugfix
  • Feature
  • Refactoring

Context

Did your issue had any of the "$" label on it?

  • Fill up your DOT address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI;

no fix

CleanShot.2023-12-22.at.11.10.22.mp4

with fix

CleanShot.2023-12-22.at.11.08.28.mp4

Copilot Summary

copilot:summary

copilot:poem

@hassnian hassnian requested a review from a team as a code owner December 22, 2023 06:04
@hassnian hassnian requested review from yangwao and removed request for a team December 22, 2023 06:04
@kodabot
Copy link
Collaborator

kodabot commented Dec 22, 2023

SUCCESS @hassnian PR for issue #8641 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

Copy link

netlify bot commented Dec 22, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 6076fac
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6593c2bf5c344400082ecde3
😎 Deploy Preview https://deploy-preview-8729--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

reviewpad bot commented Dec 22, 2023

AI-Generated Summary: This pull request introduces a key fix to the issue of video images not loading in the VideoMedia view component. The update includes adding a reference to the video element and a new event listener for 'loadeddata', facilitating the playing and pausing of the video when not in autoplay mode and the source property is defined. The patch also introduces an import from '@vueuse/core' for the 'useEventListener'. As a result, this significantly improves the handling of video display and interactivity, making for a more user-friendly, dependable media interaction.

@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Dec 22, 2023
@hassnian hassnian changed the title fix: ideos in collection are not loading #8641 fix: videos in collection are not loading #8641 Dec 22, 2023
@hassnian hassnian marked this pull request as draft December 22, 2023 06:09
@hassnian hassnian marked this pull request as ready for review December 22, 2023 06:14
@roiLeo roiLeo added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Dec 22, 2023
@prury
Copy link
Member

prury commented Dec 22, 2023

@hassnian
this one still won't load here for me nor inside or outside gallery item

image

https://deploy-preview-8729--koda-canary.netlify.app/ahk/gallery/315-9

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Dec 22, 2023
@hassnian
Copy link
Contributor Author

hassnian commented Dec 23, 2023

@hassnian this one still won't load here for me nor inside or outside gallery item

image

https://deploy-preview-8729--koda-canary.netlify.app/ahk/gallery/315-9

@prury it's working for me , wait a bit it will load eventually. that one takes some time

CleanShot 2023-12-23 at 07 58 36@2x

CleanShot 2023-12-23 at 07 59 42@2x

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (!autoPlay.value && props.src) {
useEventListener(video, 'loadeddata', () => {
video.value.play()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it will play video in milliseconds to show the thumbnail, the transferred size in the network will be higher than the canary, no?

Copy link
Contributor Author

@hassnian hassnian Dec 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the transferred size in the network will be higher than the canary, no?

yes, just tested it to make sure

@preschian got any other solution ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, capture it on metadata workers: https://github.com/kodadot/workers/blob/main/image/src/routes/metadata.ts#L74-L76. And then store it on our indexer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like first frame of video? That could be good I guess

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good Idea!

@prury
Copy link
Member

prury commented Dec 26, 2023

@prury it's working for me , wait a bit it will load eventually. that one takes some time

after waiting 20m:

image

image

it does load when i go straight to the URL:
https://image-beta.w.kodadot.xyz/ipfs/bafybeihwql6baf5t45hz7bruyfl4xqah7ei2fua6vx2g5medw5wj32lf6e
(i think this is the right image)

@hassnian hassnian changed the title fix: videos in collection are not loading #8641 fix: videos in collection are not loading Dec 28, 2023
@vikiival
Copy link
Member

@JustLuuuu does this work for u?

@JustLuuuu
Copy link
Member

@JustLuuuu does this work for u?

wfm

Copy link

codeclimate bot commented Jan 2, 2024

Code Climate has analyzed commit 6076fac and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

sonarcloud bot commented Jan 2, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@preschian preschian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this one. I don't think this is an issue. Because the item uses a placeholder on the metadata, since this is on Assethub, I think it's better for the creator to update the metadata with a proper thumbnail/image instead.

But feel free to merge it

Because if you see metadata for this item, kodadot.xyz/ahk/gallery/315-8 is using the Koda image placeholder as a poster

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hassnian
Copy link
Contributor Author

hassnian commented Jan 2, 2024

indeed this pr is only about the preview issue

@yangwao
Copy link
Member

yangwao commented Jan 4, 2024

pay 20 usd

@yangwao yangwao merged commit 1b3e227 into kodadot:main Jan 4, 2024
16 checks passed
@yangwao
Copy link
Member

yangwao commented Jan 4, 2024

😍 Perfect, I’ve sent the payout
💵 $20 @ 7.7 USD/DOT ~ 2.597 $DOT
🧗 13QUj3pZyFNfYj4AM336hRdyLQbevj5H3sR4PKmLEXLdwZhh
🔗 0xc1763598a53d7ed40eb9eca7abee2575514a3a93d83d6e5a47ac8c5ef359bc2b

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Jan 4, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paid pull-request has been paid S-changes-requested-🤞 PR is almost good to go, just some fine tunning S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved small Pull request is small waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Videos in collection are not loading
8 participants