-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve filtering dates for specific operators #3362
base: develop
Are you sure you want to change the base?
Changes from 11 commits
1cc4a83
4a9573b
6181ecc
c2a9569
d1ff23e
f7c4c51
b8fdbf5
7d9c58f
db74226
ac0b3fc
6e63c83
2419502
4caca2e
a4e8795
d66a5c6
d598c23
e9d63e4
5c04b8b
b3d2041
248ea65
3c3b197
cde383f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import {HoistBase} from '@xh/hoist/core'; | ||
import {Field, Store, FieldFilter, FieldType, genDisplayName, View} from '@xh/hoist/data'; | ||
import {isEmpty} from 'lodash'; | ||
import moment from 'moment'; | ||
import {FieldFilterOperator} from './Types'; | ||
|
||
export interface BaseFilterFieldSpecConfig { | ||
|
@@ -74,7 +75,9 @@ export abstract class BaseFilterFieldSpec extends HoistBase { | |
this.forceSelection = forceSelection ?? false; | ||
this.values = values ?? (this.isBoolFieldType ? [true, false] : null); | ||
this.hasExplicitValues = !isEmpty(this.values); | ||
this.enableValues = this.hasExplicitValues || (enableValues ?? this.isEnumerableByDefault); | ||
this.enableValues = | ||
!this.isMismatchedDateFieldType && | ||
(this.hasExplicitValues || (enableValues ?? this.isEnumerableByDefault)); | ||
} | ||
|
||
/** Full Field derived from source. */ | ||
|
@@ -116,11 +119,6 @@ export abstract class BaseFilterFieldSpec extends HoistBase { | |
return this.filterType === 'collection'; | ||
} | ||
|
||
get isDateBasedFieldType(): boolean { | ||
const {fieldType} = this; | ||
return fieldType === 'date' || fieldType === 'localDate'; | ||
} | ||
|
||
get isNumericFieldType(): boolean { | ||
const {fieldType} = this; | ||
return fieldType === 'int' || fieldType === 'number'; | ||
|
@@ -130,6 +128,20 @@ export abstract class BaseFilterFieldSpec extends HoistBase { | |
return this.fieldType === 'bool'; | ||
} | ||
|
||
get isDateBasedFieldType(): boolean { | ||
const {fieldType} = this; | ||
return fieldType === 'date' || fieldType === 'localDate'; | ||
} | ||
|
||
/** | ||
* Detects difference in date-based fieldType defined on fieldSpec and source field, indicating | ||
* custom value parsing for > and <= operators. | ||
*/ | ||
get isMismatchedDateFieldType(): boolean { | ||
const sourceField = this.source.fields.find(f => f.name === this.field); | ||
return this.fieldType === 'localDate' && sourceField.type === 'date'; | ||
} | ||
|
||
loadValues() { | ||
if (!this.hasExplicitValues && this.enableValues) { | ||
this.loadValuesFromSource(); | ||
|
@@ -149,6 +161,18 @@ export abstract class BaseFilterFieldSpec extends HoistBase { | |
); | ||
} | ||
|
||
parseDate(v: any, op: FieldFilterOperator): Date { | ||
let ret = moment(v, ['YYYY-MM-DD', 'YYYYMMDD'], true); | ||
if (!ret.isValid()) return null; | ||
|
||
// Note special handling for '>' & '<=' queries. | ||
if (['>', '<='].includes(op)) { | ||
ret = ret.endOf('day'); | ||
} | ||
|
||
return ret.toDate(); | ||
} | ||
|
||
//------------------------ | ||
// Abstract | ||
//------------------------ | ||
|
@@ -165,6 +189,7 @@ export abstract class BaseFilterFieldSpec extends HoistBase { | |
private getDefaultOperators(): FieldFilterOperator[] { | ||
if (this.isBoolFieldType) return ['=']; | ||
if (this.isCollectionType) return ['includes', 'excludes']; | ||
if (this.isMismatchedDateFieldType) return ['>', '>=', '<', '<=']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ensures the FilterChooser and GridFilter will offer the same operators for this field, which is not currently the case in the ActivityTracking example - only the GridFilter offers equals/does not equal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should be using the ActivityTracking tab as gospel for the operators we want to support - it would be better if we could support the full range of operators normally available for localDates. That said, to support e.g. |
||
return this.isValueType | ||
? ['=', '!=', 'like', 'not like', 'begins', 'ends'] | ||
: ['>', '>=', '<', '<=', '=', '!=']; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,7 +82,9 @@ const inputField = hoistCmp.factory<CustomRowModel>(({model}) => { | |
} else if (fieldSpec.isDateBasedFieldType) { | ||
return dateInput({ | ||
...props, | ||
valueType: fieldSpec.fieldType as 'localDate' | 'date' | ||
valueType: fieldSpec.isMismatchedDateFieldType | ||
? 'date' | ||
: (fieldSpec.fieldType as 'localDate' | 'date') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that if you picked a date and then closed and reopened the popover, the dateInput wouldn't render the value, due to the difference in fieldTypes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense - we need to input to be able to read |
||
}); | ||
} else if (fieldSpec.supportsSuggestions(op as FieldFilterOperator)) { | ||
return select({ | ||
|
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 believe we said we don't want to enable the Values tab in this situation, right? It could be the dev's responsibility to set enableValues to false on the fieldSpec, but I think the default behavior of not showing it for dates makes sense.
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.
It would be great to
enableValues
if we could find a clean way to restrict the values to the unique local dates in the dataset, rather than the full date timestamp. If this proves too complex however perhaps its not worth it.