-
Notifications
You must be signed in to change notification settings - Fork 176
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
change filter from is to is one of #3529
change filter from is to is one of #3529
Conversation
const newFilters = [] | ||
values.forEach(element => { | ||
const filter = { | ||
match_phrase: { | ||
[element.value]: element.label | ||
} | ||
} | ||
newFilters.push(filter); | ||
}); |
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.
nitpick: You could use Array.prototype.map
instead of Array.prototype.forEach
+ Array.prototype.push
const newFilters = values.map(element => ({
match: {
[element.value]: {
query: element.label,
type: 'phrase'
}
}
});
The match_phrase
gives problems with some app logic. Use:
match: {
[element.value]: {
query: element.label,
type: 'phrase'
}
}
filters.forEach(item => { | ||
item.params.forEach(element => { | ||
filterCustom.push({ label: element, value: item.key }) | ||
}) |
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.
nitpick: We could use Array.prototype.map
instead of Array.protype.forEach
+ Array.prototype.push
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.
The changelog is missing
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.
LGTM!
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.
LGTM!
useEffect(() => { | ||
let filterSubscriber = filterManager.getUpdates$().subscribe(() => { | ||
onFiltersUpdated(); | ||
return () => { | ||
filterSubscriber.unsubscribe(); | ||
}; | ||
}); | ||
}, []); |
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.
nitpick: The returned function to unsubscribe to the changes on the filterManager
should be the returned on the useEffect
callback, not the subscription function.
suggestion: You could use the useFilterManager
hook instead of use the filterManager.getUpdates$().subscribe
and listen to the chagens of filters
variable returned by the mentioned hook to apply the effect.
|
||
const getComponent = (item: any) => { | ||
var types = { |
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.
nitpick: You could use const
instead of var
here. Anyways, we prefer let
over var
in the variable declaration.
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.
The second revision of CR and testing: LGTM!
const prevValues = usePrevious(values); | ||
|
||
useEffect(() => { | ||
if (prevValues != values) { |
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 check for differences when values is already a dependence?
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.
done!
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.
LGTM!
Hi team, this resolve:
closes #3521