seanbehan / videojs_rails

Video JS for Rails 3.1 Asset Pipeline
91 stars 71 forks source link

support sources as an Array #35

Closed gauda closed 9 years ago

gauda commented 9 years ago

So you can give more attributes than src and type, e.g. [src:'src.mp4', type:'video/mp4', :'data-foo' => 'bar']

grzlus commented 9 years ago

I like this idea, but I'd prefer this syntax:

videojs_rails sources: { mp4: "http://domain.com/path/to/video.mp4", webm: [type: 'video/mp4', fancy_attr: :test }

In that case we will not guess (adding prefix video/ to type. It's ok for you?

grzlus commented 9 years ago

I just move logic to separate classes, this was too complicated for one file now.

If you want, you can update this merge request to new code. I'll be happy to check it. If you don't do this in near future, then I'll do it by myself.

gauda commented 9 years ago

All done. Please be so kind and review my changes.

grzlus commented 9 years ago

Code is looks good, but when you look at https://github.com/gauda/videojs_rails/blob/master/lib/videojs_rails/tags/caption.rb you can see, that style is different than in your change.

For a reason I used methods type and attributes`. In method type I put logic, how to guess type of video. Maybe in future there will be more conditions in that logic. Attributes are also a separate logic, just to prepare data, that should be in tag attributes. Law of Demeter.

gauda commented 9 years ago

I think the problem with your approach is the whitelisting of only some keys in the attributes method. When giving a hash of attributes that's not possible since you do not know which keys are given. Nevertheless I tried to come back to your style in captions.rb

grzlus commented 9 years ago

No it looks ok. There is only problem with indentation in case, but I'll change it later.

You're right. Code for captions allows to pass only some attributes to tag. I don't change it to allow more, because I only rewriten code. I don't added new features when I change helepr methods into classes.

Anyway, thanks!