-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: global default parameters #134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor issues, but otherwise looks good. A few larger things, though:
- Would love to see some tests for this
- Other global settings for imgix.js can be applied via
<meta>
tags as well, in an attempt to make the library fully configurable with HTML only. Should we apply the same principle here? What do you think?
src/ImgixTag.js
Outdated
var params; | ||
var params = {}; | ||
|
||
if (this.settings.defaultParams) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we type-check this to make sure it's an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I can do this
src/ImgixTag.js
Outdated
@@ -49,10 +49,14 @@ var ImgixTag = (function() { | |||
} | |||
|
|||
ImgixTag.prototype._extractBaseParams = function() { | |||
var params; | |||
var params = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Spaces and tabs, my friend... spaces and tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
7db7e7c
to
13bb723
Compare
@jayeb (1) was discussed offline, and I believe I've implemented (2). Could you give this another review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but there are tab/space issues all over these changes.
src/ImgixTag.js
Outdated
@@ -81,7 +83,7 @@ var ImgixTag = (function() { | |||
|
|||
if (this.settings.includeLibraryParam) { | |||
params.ixlib = 'imgixjs-' + imgix.VERSION; | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Tabs & spaces, again. Is this perchance a Prettify thing that's breaking down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah! I think it's time to tame the beast and work out what's going on here.
src/defaultConfig.js
Outdated
@@ -2,7 +2,8 @@ module.exports = { | |||
// URL assembly | |||
host: null, | |||
useHttps: true, | |||
includeLibraryParam: true, | |||
includeLibraryParam: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] T&S
* master: fix: remove support for value-less search parameters (#135)
@jayeb those pesky space and tab issues should be gone for good now! |
Description
This PR allows developers to set default parameters for every img tag. This prevents them from having to set the same parameters over and over again.
This fixes #129
Steps to test
Review new unit tests.