Skip to content
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: useSearchableViewportData - support non-text filters #2092

Merged

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Jun 20, 2024

resolves #2091

BREAKING CHANGE:

  • @deephaven/jsapi-components - The contract of useSearchableViewportData to be more consistent with useViewportData. usePickerWithSelectedValues now requires timeZone.
  • @deephaven/jsapi-utils - createSearchTextFilter requires timeZone

@bmingles bmingles changed the title useSearchableViewportData - support non-text filters feat: useSearchableViewportData - support non-text filters Jun 20, 2024
@bmingles bmingles requested a review from mofojed June 21, 2024 17:03
@bmingles bmingles marked this pull request as ready for review June 21, 2024 17:04
@bmingles bmingles force-pushed the 2091-use-searchable-viewport-data-non-text branch from 6769152 to 6bc514b Compare June 24, 2024 21:58
@@ -1825,6 +1887,11 @@ export class TableUtils {
dh.LongWrapper.ofString(TableUtils.removeCommas(value))
);
}
if (TableUtils.isDateType(type)) {
return dh.FilterValue.ofNumber(
DateUtils.trimDateTimeStringTimeZone(value) as unknown as number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work? I wouldn't expect a date String to work being passed into ofNumber (just looking at the code).

Copy link
Contributor Author

@bmingles bmingles Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I had based this on the fact that this if check wasn't there before and the fallthrough case is just passing to dh.FilterValue.ofNumber. The code I added was simply to trim the date string and no longer strip commas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, Looks like maybe this function doesn't actually get called on Date columns which would explain why it works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that block of code since it's never actually hit

@bmingles bmingles requested a review from mofojed June 27, 2024 15:39
@bmingles bmingles merged commit 7009e21 into deephaven:main Jun 27, 2024
10 checks passed
@bmingles bmingles deleted the 2091-use-searchable-viewport-data-non-text branch June 27, 2024 17:15
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants