Fix a bug from swagger-codegen #43
Conversation
| @@ -107,15 +107,15 @@ class App extends Component { | |||
| } | |||
There was a problem hiding this comment.
Github won't let me leave a comment on line 34. "search" isn't used elsewhere, so can line 34 change to
// Map from facet name to list of facet values
this.filterMap = new Map();
ui/src/App.js
Outdated
| @@ -107,15 +107,15 @@ class App extends Component { | |||
| } | |||
|
|
|||
| serializeFilters(searchFilters) { | |||
There was a problem hiding this comment.
filterMapToArray()
"""Converts filter map to array of filter strings.
Args:
filterMap: map from filter name to list of filter values
Returns:
An array of filter strings, where each string looks like "Gender=male".
ui/src/App.js
Outdated
| @@ -107,15 +107,15 @@ class App extends Component { | |||
| } | |||
|
|
|||
| serializeFilters(searchFilters) { | |||
| let filterStr = []; | |||
| let filterList = []; | |||
| searchFilters.forEach((values, key) => { | |||
| if (values.length > 0) { | |||
There was a problem hiding this comment.
Throw an error if values.length == 0, or don't check
There was a problem hiding this comment.
This is not an error case, check > uncheck will result in an initialized key corresponding to an empty array, in which case we'll just ignore it rather than provide an empty filter.
ui/src/App.js
Outdated
| @@ -107,15 +107,15 @@ class App extends Component { | |||
| } | |||
|
|
|||
| serializeFilters(searchFilters) { | |||
| let filterStr = []; | |||
| let filterList = []; | |||
There was a problem hiding this comment.
filterArr or filterArray
Python has lists but I think these are arrays in js
ui/src/App.js
Outdated
| } | ||
| } | ||
| }); | ||
| return filterStr.join(','); | ||
| return filterList; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Adding comment here so you can reply to it if you want.
I git checkout and I see this bug:
- Check a facet. Things work as expected.
- Uncheck same facet. Get console error and UI doesn't update.
There was a problem hiding this comment.
This is a separate issue which I have a followup PR staged for.
ui/src/App.js
Outdated
| @@ -31,6 +31,7 @@ class App extends Component { | |||
| }); | |||
| } | |||
| }.bind(this); | |||
| // Map of facetName:[facetValues] pairs | |||
There was a problem hiding this comment.
"pairs" is confusing -- where is the pair?
// Map from facet name to a list of facet values.
Same on line 113
ui/src/App.js
Outdated
| @@ -31,6 +31,7 @@ class App extends Component { | |||
| }); | |||
| } | |||
| }.bind(this); | |||
| // Map of facetName:[facetValues] pairs | |||
| this.searchFilters = new Map(); | |||
There was a problem hiding this comment.
Can searchFilters be renamed to filterMap?
ui/src/App.js
Outdated
| */ | ||
| filterMapToArray(filterMap) { | ||
| let filterArray = []; | ||
| filterMap.forEach((values, key) => { | ||
| if (values.length > 0) { |
There was a problem hiding this comment.
// Scenario where there are no values for a key:
// A single value is checked for a facet. The value is unchecked.
// The facet name will still be a key in filterMap, but there will be no values.
if (values.length > 0) {
The recent swagger-codegen update broke FacetedSearch. The API layer enforces that we provide queryParams as an array, not as comma separated values (which we were doing anyway) so we'll let the API layer handle that.