Combine siblings in autocomplete #8610

joshuaeilers · 2023-08-10T20:48:29Z

This PR merges autocomplete results similar to how we do it in the frontend for things like search cards.

Changes

Makes a combineSiblingsInAutoComplete and combineSiblingsInSearchResults that both share a reusable function.
Break the separate combine use cases out into their own files and ported the test for them out of siblingUtils as well. This way we can add new types that need to combine siblings without having to continue to bloat siblingUtils.
Tightened up combineSiblingEntities a little bit, using a Set, less any types, etc
The home page header / search results will combine siblings in the autocomplete including a vertical stack of platform icons, and I try to find a parentContainers set on one of the siblings to display (the case when dbt is primary but bigquery has the parentContainers we'll still show the path). This doesn't work for search cards yet, it only shows the parentContainers sometimes.
A new AutoCompleteEntityIcon component that takes care of the icon-specific rendering logic
Modifications to the autocomplete graphql response to include siblings along with parentContainers so we can display all that good stuff for each pair of siblings.

Siblings

Single

joshuaeilers · 2023-08-10T20:49:20Z

datahub-web-react/src/app/entity/shared/__tests__/siblingsUtils.test.ts

-    combineSiblingsInSearchResults,
-    shouldEntityBeTreatedAsPrimary,
-} from '../siblingUtils';
+import { combineEntityDataWithSiblings, shouldEntityBeTreatedAsPrimary } from '../siblingUtils';


This stuff was ported to combineSiblingsInSearchResults.test.ts

joshuaeilers · 2023-08-10T20:54:48Z

datahub-web-react/src/app/entity/shared/siblingUtils.ts

@@ -202,53 +203,56 @@ export const combineEntityDataWithSiblings = <T>(baseEntity: T): T => {
    return { [baseEntityKey]: combinedBaseEntity } as unknown as T;
 };

-export type CombinedSearchResult = {
+export type CombinedEntityResult = {


combineSiblingEntities is now more generalized and less about search specifically so other use cases can just pass in some entities and combine siblings (as long as you provided a gql entity that actually had siblings). Should make it more reusable.

joshuaeilers · 2023-08-10T21:09:00Z

datahub-web-react/src/app/search/utils/combineSiblingsInSearchResults.ts

+): Array<CombinedSearchResult> {
+    return combineSiblingEntities(input?.map((value) => value.entity)).map((combinedResult) => ({
+        ...combinedResult,
+        matchedFields: [],


Oversight on my part, gotta make sure this is preserved.

chriscollins3456

looking solid! a couple of minor questions and just a warning about the conflict

chriscollins3456 · 2023-08-15T16:47:26Z

datahub-web-react/src/app/entity/shared/siblingUtils.ts

@@ -2,7 +2,8 @@ import merge from 'deepmerge';
 import { unionBy, keyBy, values } from 'lodash';


looks like we have a conflict from my changes to display consistent combined search results for sibling entities in this PR: https://github.com/datahub-project/datahub/pull/8602/files#diff-dbeb733a628595000d108fe2f57b5c464d52b6da55f57d57ad4bc6e246af3914

so let's make sure we keep that functionlity! the main thing I did is simply combine the siblings entity data in combineSiblingsInSearchResults and refactored a function above it to make it reusable between its two uses

Think I got the conflict resolved 👍

chriscollins3456 · 2023-08-15T17:24:31Z

datahub-web-react/src/app/search/autoComplete/AutoCompleteEntity.tsx

+    const parentContainers =
+        entities
+            .map((ent) => {
+                const genericPropsForEnt = entityRegistry.getGenericEntityProperties(ent.type, ent);
+                const entParentContainers = genericPropsForEnt?.parentContainers?.containers || [];
+                return [...entParentContainers].reverse();
+            })
+            .find((containers) => containers.length > 0) ?? [];


I feel like this should be taken care of when we combine sibling data so we end up just getting one parentContainers based on the combined data.

In fact, with my changes in #8602 we can now get proper parentContainers combined so you can just use the combined entity here

Awesome. So like if one sibling had no parentContainers and another one missed them, it would automatically choose the one that had them present?

It does seem to work thought! Shows the bigquery containers even on the dbt primary.

yup exactly! so it'll choose the primary if both fields are filled out, but if one is filled out and not the other, it'll take the non-empty one

chriscollins3456 · 2023-08-15T17:25:31Z

datahub-web-react/src/app/search/autoComplete/AutoCompleteEntity.tsx

+    const platformNames = entities
+        .map((ent) => {
+            const genericPropsForEnt = entityRegistry.getGenericEntityProperties(ent.type, ent);
+            const platform = genericPropsForEnt?.platform;
+            if (platform?.properties?.displayName) return platform.properties.displayName;
+            if (platform?.name) return capitalizeFirstLetterOnly(platform.name) || '';
+            return '';
+        })
+        .filter(Boolean);


nit: i feel like this could be pulled out into its own function. Also I see elsewhere that we have siblingPlatforms on genericProperties that you can maybe use here?

Updated! I left the iteration in this component but reused an existing helper function we already have.

chriscollins3456 · 2023-08-15T17:29:56Z

datahub-web-react/src/app/search/SearchBar.tsx

@@ -149,6 +142,7 @@ export const SearchBar = ({
    hideRecommendations,
    showQuickFilters,
    viewsEnabled = false,
+    combineSiblings = false,


so we default want to not combine siblings in our search bar? is there somewhere where we particularly don't want them combined?

Right the idea was to make siblings an opt-in behavior, where the home/searchResults pages have that behavior but not assume that all use cases in the UI would get that behavior was my thought.

chriscollins3456

nice man looking good!

Josh Eilers added 8 commits August 9, 2023 14:24

feat(search): combine autocomplete siblings

0975526

Merge branch 'master' into feature/prd-498

60eee46

really rough rendering of siblings in the autocomplete

0116d15

icons

4aa73e1

Merge branch 'master' into feature/prd-498

844552a

combine sibling helper functions

e72aaea

move some files around

92dd8db

backout some todos

5f85017

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Aug 10, 2023

joshuaeilers marked this pull request as ready for review August 10, 2023 20:48

joshuaeilers commented Aug 10, 2023

View reviewed changes

Josh Eilers added 2 commits August 10, 2023 13:56

remove unhelpfuld default

91649e2

harden options typing

6953c63

joshuaeilers commented Aug 10, 2023

View reviewed changes

vercel bot deployed to Preview August 10, 2023 21:18 View deployment

some refactoring to support matchedFields

f6571f0

vercel bot deployed to Preview August 10, 2023 23:36 View deployment

Josh Eilers added 2 commits August 10, 2023 16:47

clarify some logic better with a better return

7707b54

move icons to top of autocomplete

931570a

vercel bot deployed to Preview August 11, 2023 16:02 View deployment

Josh Eilers added 2 commits August 11, 2023 09:27

render platform names in autocomplete

5b2395c

only show divider if we are going to show containers

4d573d7

vercel bot deployed to Preview August 11, 2023 16:59 View deployment

updates to greyes and conditional icon rendering

45b8ded

vercel bot deployed to Preview August 11, 2023 17:43 View deployment

make platform names casing correct

fe9c179

vercel bot deployed to Preview August 11, 2023 19:24 View deployment

Joshua Eilers added 2 commits August 14, 2023 12:53

Merge branch 'master' into feature/prd-498

e89afc2

remove todo

5620c5e

vercel bot deployed to Preview August 14, 2023 20:25 View deployment

chriscollins3456 reviewed Aug 15, 2023

View reviewed changes

Josh Eilers added 2 commits August 15, 2023 13:19

Merge branch 'master' into feature/prd-498

97e2b84

simplify platform rendering

7c8d0b2

vercel bot deployed to Preview August 15, 2023 21:43 View deployment

chriscollins3456 approved these changes Aug 16, 2023

View reviewed changes

joshuaeilers merged commit 7c2fd7b into datahub-project:master Aug 16, 2023

joshuaeilers deleted the feature/prd-498 branch August 16, 2023 15:53

asikowitz pushed a commit to asikowitz/datahub that referenced this pull request Aug 23, 2023

Combine siblings in autocomplete (datahub-project#8610)

a439807

yoonhyejin pushed a commit that referenced this pull request Aug 24, 2023

Combine siblings in autocomplete (#8610)

cc8b007

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine siblings in autocomplete #8610

Combine siblings in autocomplete #8610

joshuaeilers commented Aug 10, 2023 •

edited

Loading

joshuaeilers Aug 10, 2023

joshuaeilers Aug 10, 2023

joshuaeilers Aug 10, 2023

chriscollins3456 left a comment

chriscollins3456 Aug 15, 2023

joshuaeilers Aug 15, 2023

chriscollins3456 Aug 15, 2023

joshuaeilers Aug 15, 2023

joshuaeilers Aug 15, 2023

chriscollins3456 Aug 15, 2023

chriscollins3456 Aug 15, 2023

joshuaeilers Aug 15, 2023

chriscollins3456 Aug 15, 2023

joshuaeilers Aug 15, 2023

chriscollins3456 left a comment

		@@ -2,7 +2,8 @@ import merge from 'deepmerge';
		import { unionBy, keyBy, values } from 'lodash';

Combine siblings in autocomplete #8610

Combine siblings in autocomplete #8610

Conversation

joshuaeilers commented Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

joshuaeilers commented Aug 10, 2023 •

edited

Loading