phonegap / phonegap-plugin-contentsync

Download and cache remotely hosted content
Apache License 2.0
206 stars 98 forks source link

Doc issue? #18

Closed cfjedimaster closed 8 years ago

cfjedimaster commented 9 years ago

The first example shows

var sync = ContentSync.sync({ src: 'http://myserver/assets/movie-1', id: 'movie-1' });

but later the docs say that all resources should be zipped. Would it help reinforce the fact if you used a URL that makes it more clear?

i.e.

var sync = ContentSync.sync({ src: 'http://myserver/assets/movie-1.zip', id: 'movie-1' });
macdonst commented 9 years ago

Good point but as long as the endpoint produces a application/octet-stream mime type that can be verified as a zip when we download it, it doesn't really matter what the end point is.

cfjedimaster commented 9 years ago

That's fair, but I'm also of the mind of making things as obvious as possible. ;)

On Mon, May 18, 2015 at 5:56 PM, Simon MacDonald notifications@github.com wrote:

Good point but as long as the endpoint produces a application/octet-stream mime type that can be verified as a zip when we download it, it doesn't really matter what the end point is.

— Reply to this email directly or view it on GitHub https://github.com/phonegap/phonegap-plugin-contentsync/issues/18#issuecomment-103241812 .

Raymond Camden, Developer Advocate for MobileFirst at IBM

Email : raymondcamden@gmail.com Blog : www.raymondcamden.com Twitter: raymondcamden

jskrzypek commented 9 years ago

@cfjedimaster you have a good point, but this might lock in people to think that they can only point to .zip endpoints. I think it'll do the same job of making things extra obvious if there's just a short comment before or after the first line of the example. This could work

// Create a new instance of ContentSync pointing to zipped resource 'movie-1'
var sync = ContentSync.sync({ src: 'http://myserver/assets/movie-1', id: 'movie-1' });
cfjedimaster commented 9 years ago

I think more people will use something.zip then somethingThatIsAZip, but I'll not argue the point anymore. :)

On Mon, Jun 1, 2015 at 5:31 AM, Joshua Skrzypek notifications@github.com wrote:

@cfjedimaster https://github.com/cfjedimaster you have a good point, but this might lock in people to think that they can only point to .zip endpoints. I think it'll do the same job of making things extra obvious if there's just a short comment before or after the first line of the example. This could work

// Create a new instance of ContentSync pointing to zipped resource 'movie-1'var sync = ContentSync.sync({ src: 'http://myserver/assets/movie-1', id: 'movie-1' });

— Reply to this email directly or view it on GitHub https://github.com/phonegap/phonegap-plugin-contentsync/issues/18#issuecomment-107391907 .

Raymond Camden, Developer Advocate for MobileFirst at IBM

Email : raymondcamden@gmail.com Blog : www.raymondcamden.com Twitter: raymondcamden

jezmck commented 8 years ago

Please see my PR which addresses the non-obviousness that this plugin is intended for zips only.

cfjedimaster commented 8 years ago

Really surprised this isn't addressed yet. The idea of adding the comment would take two seconds. Please make it so. :)

cfjedimaster commented 8 years ago

I think you should add the comment and pull in @jezmck 's PR too. Make it really obvious.

cfjedimaster commented 8 years ago

I've made a new PR myself. I went ahead and change movie-1 to movie-1.zip just to make it more clear and added a comment for what @macdonst said.

timkim commented 8 years ago

Hi all

Thanks for the feedback and sorry for the long wait time. I merged in the docs PRs but I don't see @cfjedimaster PR. Perhaps it wasn't pushed up?

cfjedimaster commented 8 years ago

It's very possible I didn't hit the final button - checking now.

cfjedimaster commented 8 years ago

Please see now - yeah - dumb me forgot to actually finish the PR.

timkim commented 8 years ago

Ok, I believe all PRs are now merged in!