GoogleWebComponents / google-youtube-upload

Element enabling you to upload videos to YouTube.
Other
40 stars 28 forks source link

Review #1

Closed jeffposnick closed 10 years ago

jeffposnick commented 10 years ago

Big review of the whole google-youtube-upload element.

Setting up what's hopefully a correct GitHub review request with some new branches.

addyosmani commented 10 years ago

Hey @jeffposnick. Thanks for putting this together. Is it possible for future reviews to squash your commits? :) Makes it easier to review a bit.

jeffposnick commented 10 years ago

Sorry about the confusing review structure; I'm still getting the hang of the best practices for GitHub reviews.

Stay tuned for an upcoming commit to address all the comments.

jeffposnick commented 10 years ago

I've pushed a number of changes to address the review comments. The updated demo is at http://jeffposnick.github.io/google-youtube-upload/components/google-youtube-upload/demo.html

I switched over the demo.html to use a <template is="auto-binding"> approach, which worked well in general, but I'm not fully happy about how the page handles the different "state"s of the upload flow right now, with <div>s that toggle visibility based on a bound state variable. I'm curious to hear feedback about better approaches.

I did try using sub-templates that have <template bind if={{ state == 'blah' }}> set, which I think is cleaner, but then I ran into problems getting those templates to work with auto-binding.

ebidel commented 10 years ago

did try using sub-templates that have <template bind if={{ state == 'blah' }}> set,

It's possible sub-templates are a bug in <template is="auto-binding">. Did you try getting rid of bind: <template if={{ state == 'blah' }}>? Should be the same, but it may work.

ebidel commented 10 years ago

How is this coming @jeffposnick?

jeffposnick commented 10 years ago

@ebidel I'm reading through the newly updated docs to try to get a more complete picture before I can determine whether it's a limitation of templates or error on my part.

One of the things I'm trying to work around is the fact that if I include the <google-youtube-upload> in a conditional <template> that's then hidden, the event handlers defined on the <google-youtube-upload> no longer fire.