Skip to content

use variable already set for URL and remove whitespace. #20

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

Merged
merged 7 commits into from
Jul 3, 2014

Conversation

DanielOchoa
Copy link
Contributor

It seems this is the most popular way to set up meteor apps in phonegap. Have you considered making this a npm package so it can be set up with one command on a cordova project?

@zeroasterisk
Copy link
Owner

Whoa!

I guess it's time to put on my big-boy-pants and take this seriously vs. the experiment it started out as, eh?

Thanks for the PR and the rewrite...

Give me a few days to dust things off and try this out myself on a couple of platforms, but reading through it, I suspect it will work fine.

Out of curiosity what platforms have you tested this on so far?

Also you mentioned making it a npm package... I'm on board with that plan, but unsure how to tell NPM to install the JS to the shared www/js folder in cordova. recommendations?

@DanielOchoa
Copy link
Contributor Author

No problem, it's always nice to contribute. I've noticed a few blogs and apps mention that they are using this method and it seems that it works really well and that integrates with Cordova plugins without too many issues.

As you can see I'm also phasing out zepto so as to not have all that code included when meteor loads. As far as testing goes I'm only testing it on iOS for now, but I'm sure it will work fine on Android.

For the NPM idea, I already set up this repo: https://github.com/poetic/meteor-rider. It uses a node script to copy the files into the Cordova project - it also asks you for the meteor url and replaces it on the new index.html file. Since you are open to the npm idea, It's probably better to move the npm package to use your repo instead.

@DanielOchoa
Copy link
Contributor Author

It is not, good catch. I'm still working on cleaning it up. I agree that it being a property set on init is better just like I originally had it, then I realized the meteorUrl property was returning undefined inside the onSuccess function.

I just set it up that way so it works for now while I fix it. Perhaps I should have not committed it that way.

@zeroasterisk
Copy link
Owner

no problem... at least we are on the same page...

Could it be that when you set the onSuccess function you are passing in just the function, not the object.function?

In which case this refers to the request object, not MeteorRider...

@DanielOchoa
Copy link
Contributor Author

Actually, yes, that is the case.
this.onSuccess.bind(this); now allows us to reference MeteorRider when using this inside onSuccess.

zeroasterisk added a commit that referenced this pull request Jul 3, 2014
use variable already set for URL and remove whitespace.
@zeroasterisk zeroasterisk merged commit c17425e into zeroasterisk:master Jul 3, 2014
zeroasterisk added a commit that referenced this pull request Jul 3, 2014
@zeroasterisk
Copy link
Owner

@DanyHunter would you do me a solid and try out the latest release on the [dev] branch?

https://github.com/zeroasterisk/MeteorRider/tree/dev

also review the updated README -- yes, i know the image is missing because it's pathed to master :/

@DanielOchoa
Copy link
Contributor Author

For sure.

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