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

svelte media bindings #502

Merged
merged 3 commits into from
Sep 1, 2020
Merged

Conversation

jasonlyu123
Copy link
Member

@jasonlyu123
Copy link
Member Author

The readonly modifier isn't needed. It's just for documentation purposes. And it could be easiler for future works if someday we can typecheck readonly props.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Nice idea of not having it as part of the big HTMLAttributes list 👍 I suggested some lowercase changes because all props are lowercased by svelte2tsx. Also some properties which are on HTMLAttributes, like defaultplaybackrate, should be moved into this new interface I think.

packages/svelte2tsx/svelte-jsx.d.ts Show resolved Hide resolved
packages/svelte2tsx/svelte-jsx.d.ts Show resolved Hide resolved
packages/svelte2tsx/svelte-jsx.d.ts Show resolved Hide resolved
packages/svelte2tsx/svelte-jsx.d.ts Show resolved Hide resolved
@jasonlyu123
Copy link
Member Author

I have to look into it. all lowercase would have typescript errors on two way binding but camel case won't have error in both props and two way binding.

@jasonlyu123
Copy link
Member Author

jasonlyu123 commented Aug 31, 2020

two-way binding is not being transformed to all lowercase. Another problem is most of these camel case props are not HTML attributes but svelte props. The compiler would complain when you use all lower cases that also means the autocomplete is wrong.

@dummdidumm
Copy link
Member

Damn that's tricky ... About the autocompletion: We could try to turn of autocompletion for all JSX intrinsic elements, they should be shown anyway through the html language service. This would have the additional benefit of not everything showing up twice. If that works, you could just have two variants inside the JSX typings - one all lowercase for the attribute, one camelcased for the binding.

@dummdidumm
Copy link
Member

I just noticed that the fix in #496 for #494 has the same problem (so #494 is not fixed yet). My bad.

@jasonlyu123
Copy link
Member Author

What do you think about also transform two-way binding to lowercase? We can let the svelte compiler validate the casing on the binding on the svelte-specific props.

@dummdidumm
Copy link
Member

Good idea, I think this will work. It's transformed to a simple input property, so we can lowercase it like we do with the real input properties. But we should only do this for html elements, not component elements.

@jasonlyu123
Copy link
Member Author

It seems like only binding is case-sensitive that's just put two variant as you suggest earlier

@jasonlyu123
Copy link
Member Author

jasonlyu123 commented Sep 1, 2020

Some of them doesn't work when used as HTML attribute. It seems like they are svelte spefic props. So I removed the lowercase variant

<script>let playbackRate = 1</script>
<video playbackRate={1} /> <!--doesn't work -->
<video bind:playbackRate /> <!--work -->

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

👍

@dummdidumm dummdidumm merged commit 41827f7 into sveltejs:master Sep 1, 2020
@jasonlyu123 jasonlyu123 deleted the media-binding branch November 13, 2020 05:01
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