nstudio / nativescript-cardview

:diamonds: :clubs: NativeScript widget for Material Design CardView
MIT License
282 stars 51 forks source link

feat(iOS): support CardView on iOS #5

Closed NathanWalker closed 8 years ago

NathanWalker commented 8 years ago

closes https://github.com/bradmartin/nativescript-cardview/issues/2

A couple things of note:

very important: Please help test Android before merging. My local Android setup had some issues so I'd like to ensure you are able to run Android still with your setup.

bradmartin commented 8 years ago

@NathanWalker - per the {N} team plugins js files they like the .js files shipped, cause users who pull this will need the compiled .js files if they don't want to bother with the compiling/transpiling the typescript files. I'll pull this down before publishing and make sure they're included with the package, no worry. Just FYI.

Or does the build script do the ts to .js? Not looked at it yet.

I'll check over everything later and get this published ASAP, hopefully tomorrow. Thanks for your help.

NathanWalker commented 8 years ago

We definitely want the .js files shipped ;) You just don't need them on the repo. The build script will compile everything out, then you publish that (with the *.js) files, so those will always go to npm, but yeah, no need to keep .js files in the repo itself.

bradmartin commented 8 years ago

Learned something new. Very nice work indeed. I'll have to do a test run on that before shipping, I'll prob screw it up initially. This is breaking new ground for me :laughing:

NathanWalker commented 8 years ago

Oh it can be confusing, I've definitely screwed it up more than once. Biggest thing is to make sure Android still works for you as it did.

bradmartin commented 8 years ago

Okay so I synced up with this version and errors when trying to run, typescript related. Looks like it's possibly permissions issues on Windows. Any way to workaround that since we only ship the .ts files?

bradmartin commented 8 years ago

I had to actually run tns install typescript to get it running, of course to generate the .js files. So I guess the better question is should we still not include the .js files, what about users who don't know .TS or how to run tsc to generate the .js files? I don't want a new NativeScript user to get the module and then it blow up on them, it will discourage some people.

Let me know your ideas on how to fix this.

NathanWalker commented 8 years ago

So there's always 2 main points to these library/module discussions:

  1. Consumers of the module
  2. Contributors to the module
  3. Consumers need the .js files. The workflow to publish should include a build script that compiles all the .ts to .js. Then the library is published via npm publish including the .js files. We have this build script now in place.
  4. Contributors just need steps to install and setup their dev environment to run the module demo, etc. Usually can all be handled with the build script in package.json:

In this case, since our build script includes this:

tsc cardview-common.ts cardview.android.ts cardview.ios.ts

This implies a dependency on tsc, the typescript compiler. So we can either: A. mention in a CONTRIBUTING.md doc that they must npm install typescript -g (install typescript which includes tsc globallly) or... B. declare typescript as devDependencies in package.json, so when contributers run npm install after cloning the lib, they get that dependency installed out of the box.

For typescript (since it's become a common dependency for many libraries, I mean {N} entire src is written in it), then Option A. is probably the best IMO.

I don't want a new NativeScript user to get the module and then it blow up on them, it will discourage some people.

This is a non-issue because all {N} user's installing the lib via npm install nativescript-cardview will always get the .js files because they would always get published with the lib when it's published via npm publish

make sense?

NathanWalker commented 8 years ago

I'll submit a PR in a moment to clear this up... stand by...

bradmartin commented 8 years ago

Makes sense. Thanks for that. I just want it to be smooth going for devs, hope you understand my concern on this. Any issue at this point for {N} can cause a dev to say F*$% THIS and move on to another framework. Even though this is a plugin, I still don't want any lower level experienced guys to have a headache and have to start searching for the reasons why this isn't working and then they blame it on NativeScript or they think "Brad is an idiot, can't even ship a plugin". I know that's not the case but I don't want any doubt to arise from anything I've worked on in regards to {N}. I'm investing heavily in it and the better job I can do to make things smoother to help increase adoption for others I'm all about.

Apologies for the small soap box just want to express where I'm coming from on making sure this isn't an issue for more inexperienced devs.

NathanWalker commented 8 years ago

Definitely. I feel the same. This should help: https://github.com/bradmartin/nativescript-cardview/pull/6

Also, I assume you get various TypeScript warnings when running npm run build before publishing right? This is normal since {N} relies on certain platform globals that aren't referenced in a typing when compiling. We could fix this by including some typings but not sure it's really worth it at this point since it compiles out just fine.

bradmartin commented 8 years ago

yea I wouldn't worry about adding 'typings' no big deal.

I just want to get this right when I publish :question: so I normally just npm publish so this time I need to be in the repo (on cli) and what are the commands?

npm run build npm publish

is there anything else?