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

perform deep merge for Defaults.set() #4364

Closed
wants to merge 3 commits into from

Conversation

jtdevos
Copy link

@jtdevos jtdevos commented May 18, 2016

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

  • Changed Defaults.set() behavior to perform a "deep" merge, so that it is possible, for example, to change multiple ajax defaults (e.g. "ajax.dataType" and "ajax.delay") without one call overwriting the other.
  • Added test to assert new behavior

@jtdevos
Copy link
Author

jtdevos commented May 18, 2016

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
Copy link
Member

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.

@kevin-brown
Copy link
Member

Let me know if you'd like me to run grunt

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.

and then add the updated dist files to this pull request

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);
Copy link
Member

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.

@alexweissman alexweissman mentioned this pull request Oct 26, 2017
3 tasks
@alexweissman alexweissman added this to the 4.0.6 milestone Oct 26, 2017
@alexweissman
Copy link
Contributor

Merged into develop for 4.0.6 release.

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.

None yet

3 participants