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

Add option for image and video source to be a url. #153

Merged
merged 4 commits into from
Oct 12, 2022

Conversation

pvanderlaat
Copy link
Contributor

@pvanderlaat pvanderlaat commented Sep 28, 2022

Solving the first two parts of the following issue by allowing visualsource to accept a string for its "source" and turn it into an HTMLElement('img') or HTMLElement('video'):

#61

@pvanderlaat pvanderlaat changed the title Pv image source url Add option for image source to be a url Sep 28, 2022
@pvanderlaat pvanderlaat changed the title Add option for image source to be a url Add option for image and video source to be a url. Sep 28, 2022
Copy link
Collaborator

@clabe45 clabe45 left a comment

Choose a reason for hiding this comment

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

Looks great!! I requested a few minor changes, but after that it's ready to be merged. Thank you!

class Video extends AudioSourceMixin(VisualSourceMixin(Visual)) {
constructor (options: VisualSourceOptions) {
if (typeof (options.source) === 'string') {
const img = document.createElement('video')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be video

@@ -25,7 +25,7 @@ interface VisualSource extends Visual {
}

interface VisualSourceOptions extends VisualOptions {
source: HTMLImageElement | HTMLVideoElement
source: HTMLImageElement | HTMLVideoElement | string
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you revert this line and override source in ImageOptions and VideoOptions, will it still compile?

Updating ImageOptions to this:

interface ImageOptions extends VisualSourceOptions {
  source: HTMLImageElement | string
}

If it doesn't compile, it's fine to leave it how it is.

@clabe45 clabe45 merged commit 422220f into etro-js:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants