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
perform deep merge for Defaults.set() #4364
Conversation
I notice that the build failed - I assume it's because I failed to run grunt prior to committing. Let me know if you'd like me to run grunt and then add the updated dist files to this pull request. |
assert.equal( | ||
defaults.defaults.ajax.delay, | ||
ajaxDelay |
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.
You're missing a comma here, and that is failing the build.
Running grunt will run the automated tests and confirm that the files can be properly compiled. The CI will automatically do this, so it's not required, but it's helpful to ensure the build doesn't fail when we test it.
This isn't required (and I usually don't encourage it). The CI will automatically compile the files before testing, and including the dist changes usually ends with unnecessary merge conflicts later. |
var dataDataType = 'xml' | ||
defaults.set('ajax--delay', ajaxDelay); | ||
defaults.set('ajax--dataType', dataDataType); |
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.
This should really be ajax--data-type
, to match the HTML attribute.
Merged into |
This pull request includes a
The following changes were made