-
Notifications
You must be signed in to change notification settings - Fork 14.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
feat: list view filters for Query History #11702
Conversation
56ad4aa
to
6f2bf18
Compare
Codecov Report
@@ Coverage Diff @@
## master #11702 +/- ##
==========================================
- Coverage 67.56% 63.74% -3.83%
==========================================
Files 918 924 +6
Lines 44755 44827 +72
Branches 4237 4253 +16
==========================================
- Hits 30240 28575 -1665
- Misses 14412 16075 +1663
- Partials 103 177 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
849fbd5
to
abcc71f
Compare
b32d729
to
45565b9
Compare
343c17e
to
346f825
Compare
@@ -128,7 +132,7 @@ function QueryList({ addDangerToast, addSuccessToast }: QueryListProps) { | |||
...commonMenuData, | |||
}; | |||
|
|||
const initialSort = [{ id: 'changed_on', desc: true }]; | |||
const initialSort = [{ id: 'start_time', desc: true }]; |
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.
nit: make this an enum initSortId
at the top
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 converted all the column names to an enum
.filter(f => { | ||
if (typeof f.value === 'undefined') return false; | ||
if (Array.isArray(f.value) && !f.value.length) return false; | ||
return true; |
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.
how about
.filter(f => !((typeof f.value === 'undefined') || (Array.isArray(f.value) && !f.value.length)))
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.
Do you actually find that more readable? These are two separate conditions for filtering out a value, it makes sense to keep them seperate imo
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 do, since you're trying to see if it has a value on each type, but it's just preference. It mainly feels redundant to return an explicit boolean for a boolean conditional.
How about something like this for readability?
let hasValue = true;
hasValue = !(typeof f.value === 'undefined');
hasValue = !(Array.isArray(f.value) && !f.value.length);
return hasValue;
if that doesn't feel like an improvement, we can go with what you have. It's not anything major.
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.
ok, updated. I have less of an issue with it now after the prettier formatting.
Header: t('Search by query text'), | ||
id: 'sql', | ||
input: 'search', | ||
operator: 'ct', |
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.
second @hughhhh's comment on these strings, too.. they could prob all benefit from enums as well.
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.
Created an enum FilterOperators
with more descriptive names for each operator.
346f825
to
d678727
Compare
@@ -89,3 +99,24 @@ export interface FetchDataConfig { | |||
export interface InternalFilter extends FilterValue { | |||
Header?: string; | |||
} | |||
|
|||
export enum FilterOperators { | |||
starts_with = 'sw', |
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.
could we make this snakeCase?
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.
converted to camelCase since that's the js standard
765da29
to
4475013
Compare
merging this before I get any more comments! 😃 |
SUMMARY
antd
andmoment
due to incompatible versions in the types for the DatePickerBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
TEST PLAN
ADDITIONAL INFORMATION