Skip to content
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

added callback option #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

godspeedelbow
Copy link

In our situation we don't want the user to go the app store but we rather provide custom callback behavior that's called when the app is not found

I also added a trailing ; after end in the android url because our Titanium app goes haywire without it.

Also, switched package and scheme for same reason

@godspeedelbow
Copy link
Author

I wasn't aware that when I make subsequent commits after creating a pull request, Github just adds them to the PR. Anyways, this PR gets a bit convoluted like this

@hampusohlsson
Copy link
Owner

Great suggestion. Haven't been able to verify, but code looks like it should do the job :)
However, specifying a callback should perhaps take precedence over fallback link. So I suggest changing the order of the if statement. What do you think?

if (settings.callback) {
    timeout = setTimeout(settings.callback, settings.delay);
} else if (settings.fallback) {
    timeout = setTimeout(openAppStore(Date.now()), settings.delay);
}

@godspeedelbow
Copy link
Author

I had it like that before but I figured I didn't want to overrule current behavior. But now that that's an option, how about we let fallback take different values:

  • 'openAppStore', opens app store (default setting, works as it does now)
  • false, don't do anything (works as it does now)
  • function() {}, call this function as a callback

@hampusohlsson
Copy link
Owner

Yep, sounds good!

@hampusohlsson
Copy link
Owner

The open() function is starting to get a little bloated. Perhaps we can refactor and move the settings.fallback logic to its own function?

@godspeedelbow
Copy link
Author

Yeah, good idea!

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