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

Minor fixes to new media bindings #837

Closed
wants to merge 2 commits into from

Conversation

martinandert
Copy link
Contributor

Thanks for merging #819, even though it wasn't perfect! Here are a two small fixes to things I've noticed when looking at d75ab85 or when playing around with the new release.

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

Thanks, good catch on the logical expression. I don't understand why we need the if (!ranges) guard?

@@ -103,6 +103,7 @@ export function toNumber(value) {
}

export function timeRangesToArray(ranges) {
if (!ranges) return [];
Copy link
Member

Choose a reason for hiding this comment

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

when could this be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the src attribute may not be present and then all the ranges attributes return undefined.

Copy link
Contributor Author

@martinandert martinandert Sep 10, 2017

Choose a reason for hiding this comment

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

https://svelte.technology/repl?version=1.39.0&gist=66b93597de37579256dc333177c56e96

I could wrap the #each in an #if, but I don't really like to do that.

And since the method name is suffixed with ToArray, I somehow expect it to always return an array, regardless of the input.

Copy link
Member

Choose a reason for hiding this comment

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

In what browsers is that true? In Chrome:

screen shot 2017-09-10 at 6 32 41 pm

Fairly certain that's spec.

Copy link
Member

Choose a reason for hiding this comment

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

I think what's happening here is we need to initialise the bindings to an empty array — adding the guard to this function probably won't fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to initialise the bindings to an empty array

Ah ok, I can do that.

@martinandert
Copy link
Contributor Author

martinandert commented Sep 10, 2017

I think what's happening here is we need to initialise the bindings to an empty array

Done in bfaab99. Though I'm not entirely sure this is better. Judging from your screenshot, how can audio.buffered not return a TimeRanges instance in the first place?

@Rich-Harris
Copy link
Member

That's not quite what I meant — the binding needs to be initialised, i.e. the value of played needs to be set to an empty array immediately. Unfortunately that's slightly tricky as things stand, and will require some additional work (though that work will have benefits in other places), which I'll create a new issue for. Apologies for the confusion, I didn't immediately realise this wasn't possible.

The workaround in the meantime is to initialise the value manually (note the "played": [] in the bottom right).

I cherry-picked the first commit into #841, so I'll close this. Thanks.

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.

2 participants