Skip to content

Added test option #152

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

Closed
wants to merge 1 commit into from
Closed

Conversation

dsherret
Copy link

@dsherret dsherret commented Feb 5, 2015

Regarding issue #149, I added a test property to the options. Set it to true to always show the banner on load (for testing purposes). I didn't update the documentation because that can easily be updated to the way you guys like it.

I followed how things are currently done in the code; however, there's some unnecessary assignment going on. You might as well assign the property in options straight to the variable in this code:

this.defaultOpts.bgColor = options.bgColor;
this.defaultOpts.color = options.color;
this.defaultOpts.lowerThan = options.lowerThan;
this.defaultOpts.languagePath = options.languagePath;
this.defaultOpts.test = options.test;

bkgColor = this.defaultOpts.bgColor;
txtColor = this.defaultOpts.color;
cssProp = this.defaultOpts.lowerThan;
languagePath = this.defaultOpts.languagePath;
test = this.defaultOpts.test;

I think the main intent was something like this though:

bkgColor = options.bgColor || this.defaultOpts.bgColor;
txtColor = options.color || this.defaultOpts.color;
cssProp = options.lowerThan || this.defaultOpts.lowerThan;
languagePath = options.languagePath != null ? options.languagePath : this.defaultOpts.languagePath;
test = options.test || this.defaultOpts.test;

Also, these variables are global. They're not private. I'm going to go through the code and clean it up.

Additionally, the code needs to be minified... I'm not sure what minifier you used so I didn't do it (it would be nice to have a build script)

This was referenced Feb 5, 2015
@D1plo1d
Copy link

D1plo1d commented Oct 15, 2015

+1. I'd like to be able to control which browsers we support more finely.

@DawnPaladin
Copy link

+1. "Test" might not be the best name for this; it's useful to be able to activate the popup according to criteria of my choosing. Maybe name it "manualActivate" or "forceActivate" instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants