Skip to content

Extend jQuery with serializeJSON function properly #17

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 2 commits into from
Closed

Extend jQuery with serializeJSON function properly #17

wants to merge 2 commits into from

Conversation

brandonb927
Copy link

Some other fixes in this commit:

  • Fix spelling error in README
  • Update LICENSE year
  • Update Google Closure Compiled version

No core change, tests pass.

Fix spelling error

Update LICENSE year

Update Google Closure Compiled version
Regex taken from issue #6 - #6
@macek
Copy link
Owner

macek commented Mar 10, 2014

@brandonb927 my main concern with this PR is that it addresses multiple concerns in a single commit. And I can't bump the version until we have a regression test to support your findings.

You are correct that serializeJSON has not been properly exposed to the jQuery plugin. The foolish way I decided to avoid front-end testing allowed this to slip through the cracks.

I think I have arrived at a decision for the issue about supporting . and - in the field keys, too. The plugin will likely define defaults as $.jquerySerializeObjectDefaults. End users will be able to override those defaults in order to support their custom needs.

I'm addressing a couple of things with this plugin today. I'll keep you updated.

@macek
Copy link
Owner

macek commented Mar 11, 2014

@brandonb927 thanks again for reporting this issue. It has been fixed with the latest release. I strongly encourage you to revisit the source code as as much of the structure of the plugin has changed.

There are pitfalls with doing server-side testing for a front-end plugin, and you discovered one of them. I took great care to ensure this kind of bug can't happen again 😄

As for the . and - in field keys, that will be addressed in the near future.

@macek macek closed this Mar 11, 2014
@brandonb927
Copy link
Author

Thanks for commenting on this :) I squashed my commits into a single commit so there wasn't 10 commits to deal with merging. I'll take a look at the source again when it's been updated for the hyphenated fields. Awesome work!

@macek
Copy link
Owner

macek commented Mar 11, 2014

@brandonb927 please see this comment I made on another issue.

There's also an example in the README now for using hyphens with this plugin.

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.

2 participants