-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve variant and product autocomplete functions flexibility with Ransack #4767
Improve variant and product autocomplete functions flexibility with Ransack #4767
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.
Awesome, @RyanofWoods!
Note, the |
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.
Nice work and thanks for also tackling the variantAutocomplete 👏🏻
I still think we can improve the name of the option, to make it more intuitive.
See next commit.
From this change, when initializing a productAutocomplete, a function can be given to the searchParameters option argument. This callback should return an object (hash), and accepts a parameter which is the currently typed string in the autocomplete select2. The "q" key on the returned hash will be given to Ransack.
From this change, when initializing a variantAutocomplete, a function can be given to the searchParameters option argument. Before, variantAutocomplete accepted an object (hash) searchOptions argument, now instead, searchParameters should be used and given a function that returns an object (hash). It accepts a parameter which is the currently typed string in the autocomplete select2. The "q" key on the returned hash will be given to Ransack if the variant_search_term key is overridden with a falsy value. For compatibility, if searchOptions was being used, searchParameters can just return the same hash the same behavior.
For Api::VariantsController#index to use the q parameter with Ransack, variant_search_term has to be a falsy value/not exist. By default, variantAutocomplete has variant_search_term defined and needs to be overriden with a falsy value if ransack wants to be used.
b969610
to
e2f7fca
Compare
I pulled a spec change out into an new commit and changed other commits to rename |
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.
Thanks 🙏🏻
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.
Hey @RyanofWoods! This is great but I have a request: what if we try to make this change backward compatible converting the previous input format for variantAutocomplete
into the new format programmatically and print a deprecation warning so that people have the opportunity to change it?
I can do that @kennyadsl, when you mean |
Yes, I think that the console warning, combined with an entry in our CHANGELOG would be enough to let people know. We do not have a specific API for that, despite we did something similar in the past. |
Looks like we're not adding manual changes to the changelog anymore @kennyadsl and @waiting-for-dev, so I removed them and left them below for reference for the guides 👍
variantAutocomplete ChangesDEPRECATION:
Deprecated usage: $('#id').variantAutocomplete({
suppliable_only: true
}) New usage: $('#id').variantAutocomplete({
searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } }
}) |
39fc037
to
fd61689
Compare
So in whole, I added one squash commit for ease of review based on @kennyadsl's comments |
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.
Thanks @RyanofWoods, left one more last suggestion.
backend/app/assets/javascripts/spree/backend/variant_autocomplete.js
Outdated
Show resolved
Hide resolved
Make variantAutocomplete backwards compatible. Users can still pass an object of parameters, null or nothing, if the former is done, a console.warn deprecation warning is shown.
fd61689
to
5cd3007
Compare
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.
Nice! I removed the release:major
label, we can merge this in a minor now. Thanks!
Didn't want the squash commit squashed? 🤔 |
Oh, sorry, I missed that 🤦 |
No worries! I could have said something @waiting-for-dev |
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Instead, pass only searchParameters: function (_selectSearchTerm) { return { suppliable_only: true } } Ref solidusio#4767
Summary
From this change, when initializing a productAutocomplete, a callback can be given to the searchCallback option. This callback should return a hash object that will be used to merge onto the existing parameters in the select2 ajax function. The callback also allows for one argument, which is the string typed in the select2 (term). Custom Ransack can be achieved by defining this option with the Ransack query on the
q
key.Similar behavior was already existing on the
variantAutocomplete
, however accepted a simplehash
object instead of a function, and so one didn't have access to the select2 search term, so this was improved.Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):- [ ] I have attached screenshots to demo visual changes.- [ ] I have opened a PR to update the guides.- [ ] I have updated the README to account for my changes.Autocompletes are still working:
Variant
Orders#index
Orders#edit
Product
Promotion Rules - Product
Promotion Rules - Option Values