-
Notifications
You must be signed in to change notification settings - Fork 141
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(fsbazaarvoice): Expand review data source #1061
Conversation
Looks like the test for FSNetwork is failing and it doesn't seem related to anything I touched. |
Include: 'Products', | ||
Stats: 'Reviews', | ||
Limit: query.limit || 10 | ||
Limit: query.limit || 10, | ||
...(query.sort ? {Sort: query.sort} : {}) |
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.
Why not just make it Sort: query.sort
? Is it shoving in "Sort=undefined" that way?
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.
I didn't think to check to see if qs omits undefined parameters.
I just checked and qs indeed omits them, so I can change this.
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.
Seems good.
fsbazaarvoice was upgraded in brandingbrand#1061 This makes the other packages use the same version.
fsbazaarvoice was upgraded in brandingbrand#1061 This makes the other packages use the same version. brandingbrand-source-id: b3111a290fe6c0b0c9b880cf4e66501dadde65d5
Added new optional params to the bazaarvoice review data source while leaving existing functionality in place.
Filter allows for multiple values following the "Filter=first&Filter=second&Filter=third" format.
Found no existing framework to create a test case in flagship. In the interest of time, I tested it directly within the CVS app.