Skip to content

query-builder: typeahead for filter and post-filter #1381

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

Merged
merged 6 commits into from
Aug 12, 2022

Conversation

MauricioUyaguari
Copy link
Member

@MauricioUyaguari MauricioUyaguari commented Aug 4, 2022

Summary

Fixes #1382

typeAhead.mp4

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2022

🦋 Changeset detected

Latest commit: 41c834b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 21 packages
Name Type
@finos/legend-art Patch
@finos/legend-application-query Minor
@finos/legend-application Patch
@finos/legend-application-query-bootstrap Patch
@finos/legend-application-studio-bootstrap Patch
@finos/legend-application-studio Patch
@finos/legend-application-taxonomy-bootstrap Patch
@finos/legend-application-taxonomy Patch
@finos/legend-extension-application-studio-management-toolkit Patch
@finos/legend-extension-application-studio-query-builder Patch
@finos/legend-extension-dsl-data-space Patch
@finos/legend-extension-dsl-diagram Patch
@finos/legend-extension-dsl-persistence Patch
@finos/legend-extension-dsl-text Patch
@finos/legend-extension-external-language-morphir Patch
@finos/legend-extension-external-store-service Patch
@finos/legend-manual-tests Patch
@finos/legend-extension-dsl-persistence-cloud Patch
@finos/legend-application-query-deployment Patch
@finos/legend-application-studio-deployment Patch
@finos/legend-application-taxonomy-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label Aug 4, 2022
@MauricioUyaguari MauricioUyaguari self-assigned this Aug 4, 2022
@MauricioUyaguari MauricioUyaguari changed the title first draft of fuzzy search Type Ahead for Filter and Post Filter Aug 4, 2022
@MauricioUyaguari MauricioUyaguari force-pushed the fuzzy branch 3 times, most recently from 43a0239 to e1d939c Compare August 9, 2022 14:54
@MauricioUyaguari MauricioUyaguari marked this pull request as ready for review August 9, 2022 14:55
@MauricioUyaguari MauricioUyaguari force-pushed the fuzzy branch 2 times, most recently from ba4f34e to c69b6ef Compare August 9, 2022 15:06
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #1381 (41c834b) into master (0a99cab) will increase coverage by 0.21%.
The diff coverage is 59.39%.

@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
+ Coverage   41.89%   42.10%   +0.21%     
==========================================
  Files        1176     1179       +3     
  Lines       52484    52643     +159     
  Branches    11922    11966      +44     
==========================================
+ Hits        21987    22167     +180     
+ Misses      30422    30400      -22     
- Partials       75       76       +1     
Impacted Files Coverage Δ
...src/components/QueryBuilderFetchStructurePanel.tsx 43.85% <ø> (ø)
...ery/src/components/QueryBuilderPostFilterPanel.tsx 12.02% <0.00%> (-0.30%) ⬇️
.../legend-art/src/components/CustomSelectorInput.tsx 12.82% <ø> (ø)
...cation-query/src/stores/QueryBuilderFilterState.ts 47.91% <14.28%> (-1.47%) ⬇️
...on-query/src/stores/QueryBuilderPostFilterState.ts 41.71% <15.38%> (-1.06%) ⬇️
...omponents/shared/BasicValueSpecificationEditor.tsx 12.61% <17.64%> (+0.66%) ⬆️
...on-query/src/stores/QueryBuilderTypeaheadHelper.ts 63.26% <63.26%> (ø)
...n-query/src/components/QueryBuilderFilterPanel.tsx 56.95% <75.00%> (+0.90%) ⬆️
...lication-query/src/stores/QueryBuilderTestUtils.ts 95.00% <95.00%> (ø)
...uery/src/stores/QueryBuilderFetchStructureState.ts 100.00% <100.00%> (ø)
... and 28 more

@akphi akphi changed the title Type Ahead for Filter and Post Filter query-builder: typeahead for filter and post-filter Aug 10, 2022
Copy link
Contributor

@akphi akphi left a comment

Choose a reason for hiding this comment

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

Quite an elegant solution! Besides most of the suggested refactor, I think typeahead should be treated as one word.

noOptionsMessage={noOptionsMessage}
/>
) : (
<input
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of styling, from the video, I notice the discrepancy in height of the custom selector component and the height of its container causes a jump when users input, let's fix that.

Also, I think we should consider whether we should show the caret down here as it suggests auto-complete is available as people type and it's sort of distracting. What we want to do here is just let people have the feeling like they are typing some text value per usual, and happen to have these suggestions popping up.

@MauricioUyaguari MauricioUyaguari force-pushed the fuzzy branch 3 times, most recently from cad7f0e to 6ace4d2 Compare August 10, 2022 20:27
@MauricioUyaguari MauricioUyaguari force-pushed the fuzzy branch 5 times, most recently from 2013830 to b1f2bea Compare August 11, 2022 14:43
@MauricioUyaguari MauricioUyaguari force-pushed the fuzzy branch 2 times, most recently from 72e00f4 to cde8767 Compare August 11, 2022 17:53
akphi
akphi previously approved these changes Aug 12, 2022
Copy link
Contributor

@akphi akphi left a comment

Choose a reason for hiding this comment

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

Nice stuff! I left some comments, let's address them after merging.

@@ -604,6 +684,16 @@ export const BasicValueSpecificationEditor: React.FC<{
className?: string | undefined;
setValueSpecification: (val: ValueSpecification) => void;
resetValue: () => void;
selectorConfig?:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely convinced of this approach, I think what we should do is just let people pass in a function to return Promise<string[]> and we keep all the other states (loading, results) as part of this component or so.

@@ -466,6 +474,26 @@ const QueryBuilderPostFilterConditionEditor = observer(
node.condition.operator.getDefaultFilterConditionValue(node.condition),
);
};
const debouncedTypeaheadSearch = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to simplify the API of BasicValueSpecification further here. Also, I just thought of a case I don't know if we need to handle, that is what happens if we do typeahead search and the returns are booleans or numbers?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can further enhance that for numbers.
Maybe not yet for booleans.

@akphi akphi merged commit 7a892e6 into finos:master Aug 12, 2022
@akphi akphi deleted the fuzzy branch August 12, 2022 04:43
@MauricioUyaguari MauricioUyaguari mentioned this pull request Aug 23, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-present CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fuzzy-search/auto-suggestion/auto-complete in filter and post filter
2 participants