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

feat: add global default params #72

Merged

Conversation

Duder-onomy
Copy link
Collaborator

I wanted to be able to specify some default params from the applications config. Specifically, I wanted the auto='format' param. While implementing, I noticed the other global config options were untested so I added some tests for those while I was in there.

Description

  • Adds tests for current global configuration options [debug, classNames, disableLibraryParam]
  • Adds tests and feature for NEW global configuration option called defaultParams which is an object of default params that will be applied to all src's.
  • Adds notes to the readme.

New Feature

  • [NA] If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
  • [✔️] Run unit tests to ensure all existing tests are still passing
  • [✔️] Add new passing unit tests to cover the code introduced by your PR
  • [✔️] Update the readme
  • [✔️] Add some steps so we can test your cool new feature!
  • [✔️] The PR title is in the conventional commit format.

Steps to Test

  • Set a imgix param via the applications config, something like:
///
APP: {
    imgix: {
        path: ...,
        defaultParams: {
            auto: 'format',
        },
    },
},
///
  • Observe that param being applied to all src's.
  • Pass a param via the options hash or as params on the url.
  • Observe that all other param setting methods take precedence over the global values.

Greg Larrenaga added 3 commits February 4, 2019 12:52
…o that it can be overwritten after the module is parsed

Adds tests for overwriting the imgix-image component's default class
…config

Adds tests for disabling/enabling the debug param from the application config
…nfig

Adds tests for using them and how they will be merged into the src
Adds notes to readme
@frederickfogerty
Copy link
Contributor

Hey @Duder-onomy I'm currently taking a hiatus from imgix as I complete my university degree - I'm aiming to be back on board sometime later this year. Until that point, Jason (@jayeb) from imgix is the man you should reach out to for PR reviews or other questions!

Anywho, I've given this a look over and it seems fine to me. @jayeb or @sherwinski should probably give this a review as well before it's merged, as they're more in-sync with imgix's internal plans.

Copy link
Contributor

@frederickfogerty frederickfogerty left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

This looks good, but I do have a question about the fallback for setting classes.

@@ -339,6 +341,10 @@ export default Component.extend({
return placeholderURL;
}),

elementClassNames: computed('config.APP.imgix.classNames', function() {
return config.APP.imgix.classNames || 'imgix-image';
Copy link

Choose a reason for hiding this comment

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

Would there ever be a case where someone would want no default class on the img element? This || 'imgix-image' would make it impossible to unset this option entirely.

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.

4 participants