-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add attribute value filter #27
Add attribute value filter #27
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
3a31886
to
3b3fa46
Compare
d5066cf
to
3b3fa46
Compare
/retest |
fe02e6b
to
255a62f
Compare
9509170
to
aa3a174
Compare
aa3a174
to
1dca3a4
Compare
/retest |
/retest-required |
1dca3a4
to
edc525b
Compare
edc525b
to
940d197
Compare
return undefined; | ||
} | ||
|
||
const unknownFilter = severity.has('unknown') |
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.
Is it necessary to handle this unknown
case separately rather than inside the flatMap
below?
940d197
to
9537433
Compare
/retest |
ff54f2c
to
6769f1c
Compare
src/hooks/useURLState.ts
Outdated
history.push(`${location.pathname}?${queryParams.toString()}`); | ||
}; | ||
|
||
const setValuesFromParams = (newQueryParams: URLSearchParams) => { |
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.
For my comment #27 (review), rather than just renaming the parameter, I was wondering if you could avoid passing in a parameter at all and use queryParams
here? Or maybe queryParams
won't yet be set when the function is first called?
Taking it a bit further, would something like this work?
useEffect(() => {
const queryValue = queryParams.get(QUERY_PARAM_KEY) ?? initialQuery;
const tenantValue = queryParams.get(TENANT_PARAM_KEY) ?? DEFAULT_TENANT;
const showResourcesValue =
queryParams.get(SHOW_RESOURCES_PARAM_KEY) ?? DEFAULT_SHOW_RESOURCES;
setQuery(queryValue);
setTenant(tenantValue);
setAreResourcesShown(showResourcesValue === '1');
setFilters(filtersFromQuery({ query: queryValue, attributes }));
}, [queryParams])
And then you wouldn't need the other two useEffect
s at all.
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.
Yes, having the useEffect listening into the history rather than the queryValue
allowed that the text input didn't loose the caret position.
But in fact this was an issue in the input and shouldn't be solved in the useURLState hook. I changed it to use just one useEffect and adjusted the input proxying the prop in an internal state so the caret position is kept while the url parameter changes.
6769f1c
to
059bf2a
Compare
059bf2a
to
acd4bca
Compare
@jgbernalp: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
These changes introduce filters based on user UI selections. Using a LogQL parser the console plugin is able to generate UI filters from a query and vice versa.
Part of this PR:
Screen.Recording.2022-09-26.at.15.55.46.mov
Completes: https://issues.redhat.com/browse/LOG-2983 & https://issues.redhat.com/browse/OU-69