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

Embedding videos using Opencast IDs #1174

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

geichelberger
Copy link
Contributor

Previously, embedding videos was only possible using the Tobira ID. This change introduces the ability to embed videos with Opencast IDs using the same embedding format (/~embed/!v/:<opencast-id>). This enhancement improves flexibility and consistency in video embedding options.

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Jun 3, 2024
Copy link

github-actions bot commented Jun 3, 2024

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

That feature makes sense to us!

Looking at the code: could you try to avoid the duplication of code in the GraphQL queries? You can use a fragment for that. Check routes/Video.tsx and its eventFragment for orientation. I think it should be straight forward? But if you encounter weird problems, feel free to reach out.

Previously, embedding videos was only possible using the Tobira ID. This
change introduces the ability to embed videos with Opencast IDs using
the same embedding format (`/~embed/!v/:<opencast-id>`). This
enhancement improves flexibility and consistency in video embedding
options.
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Jun 3, 2024
@geichelberger
Copy link
Contributor Author

I did not think about the fragment feature; I introduced a fragment and rebased the PR onto the current master.

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Just a few comments, otherwise good to go

frontend/src/routes/Embed.tsx Show resolved Hide resolved
frontend/src/routes/Embed.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Embed.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Embed.tsx Outdated Show resolved Hide resolved
- Remove duplicate embed player code
- Make useFragment typesafe
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Thanks!

@LukasKalbertodt LukasKalbertodt merged commit 3ea80fc into elan-ev:master Jun 5, 2024
3 checks passed
@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants