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

add event hooks to smart app banner #97

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

Conversation

lendormi
Copy link

the event only hook close and hide function, extend it to all method call in smartBanner class.

For few use cases we need to call an event hook only on install call and not with close event hook.

Copy link
Collaborator

@TSMMark TSMMark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. This obviously would be a breaking change which is unfortunate. Is there a way to keep backwards-compatibility? Perhaps we can continue to call this.options.close, but add an argument which is the action that triggered the close.

For example, the hide callback might look like this:

hide: function () {
  root.classList.remove('smartbanner-show');

  if (typeof this.options.close === 'function') {
    return this.options.close('hide');
  }
},

@lendormi
Copy link
Author

I understand. I'm sorry i didn't think to keep backwards-compatibility.

  • I do some changes to backwards-compatibility.
  • I change hook name for better and semantically understanding too

however, i think the callback like this is not good semantically:

if (typeof this.options.close === 'function') {
    return this.options.close('hide');
}

i suggest to keep it simple and deprecated theses callbacks:

if (typeof this.options.hookHide === 'function') {
  return this.options.hookHide(this.type);
} else if (typeof this.options.close === 'function') { //deprecated: backwards-compatibility
  return this.options.close();
}

After somes changes in the main part of the code, it look like this :

//backwards-compatibility with old version below tag 1.4.0
if (typeof this.options.close === 'function' && this.options.hookClose === null) {
  this.options.hookClose = this.options.close;
}
if (typeof this.options.show === 'function' && this.options.hookShow === null) {
  this.options.hookShow = this.options.show;
}
// ...
if (typeof this.options.hookHide === 'function') {
  return this.options.hookHide(this.type);
} else if (typeof this.options.hookClose === 'function') { //deprecated: backwards-compatibility
  return this.options.hookClose(this.type);
}

For example :

new SmartBanner({
    daysHidden: 15,   // days to hide banner after close button is clicked (defaults to 15)
    daysReminder: 90, // days to hide banner after "VIEW" button is clicked (defaults to 90)
    appStoreLanguage: 'us', // language code for the App Store (defaults to user's browser language)
    title: 'MyPage',
    author: 'MyCompany LLC',
    button: 'VIEW',
    store: {
        ios: 'On the App Store',
        android: 'In Google Play',
        windows: 'In Windows store'
    },
    price: {
        ios: 'FREE',
        android: 'FREE',
        windows: 'FREE'
    }
    // , theme: '' // put platform type ('ios', 'android', etc.) here to force single theme on all device
    // , icon: '' // full path to icon image if not using website icon image
    // , force: 'ios' // Uncomment for platform emulation
    hookInstall: function(type){},
    hookShow: function(type){},
    hookHide: function(type){},
    hookClose: function(type){},
});

@TSMMark TSMMark requested a review from dy January 11, 2018 13:32
DRALANTONISAINANA added 3 commits June 19, 2018 11:30
the event only hook close and hide function, extend it to all method call
in smartBanner class
features:
- to keep backwards-compatibility for options.close and options.show
- use hook name function for better understanding
- include argument type to trigger os type

note: update smart-app-banner on last node_modules version
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