feat(ui) Display combined sibling results in search + 2 minor updates #8602
Conversation
| const uniqueOwnerUrns = new Set(); | ||
| const uniqueOwners = owners?.filter( | ||
| (owner) => !uniqueOwnerUrns.has(owner.owner.urn) && uniqueOwnerUrns.add(owner.owner.urn), | ||
| ); |
There was a problem hiding this comment.
Good use of a Set 👍
Optional: Maybe a useUniqueOwners(owners) custom hook? Lots of logic already in this file.
There was a problem hiding this comment.
hm why hook as opposed to a utility function? since it's not using any hooks itself - I could just do getUniqueOwners in a utility file
|
|
||
| return ( | ||
| <PreviewContainer data-testid={dataTestID} onMouseDown={onPreventMouseDown}> | ||
| <PreviewContainer data-testid={dataTestID} onMouseDown={onPreventMouseDown} id={`entityUrn-${urn}`}> |
There was a problem hiding this comment.
Pretty sure it's a good practice to keep id's unique on the page. I this with setup, it'll render this DefaultPreviewCard along with a SearchResult at the same time?
Looking at containers search result rendering here: https://github.com/joshuaeilers/datahub/blob/779d692cdc7a74ff85f33960ff7572521736329b/datahub-web-react/src/app/entity/container/ContainerEntity.tsx#L140
and then again down below in the SearchResultList.
I could be wrong here about the multiple times, but it might be a good idea to put this into a class or other data-* attribute so we don't collide.
There was a problem hiding this comment.
ah yeah good call they would definitely both be rendered with the same ID. will make that change to a class or something else. I really only think we need one of them anyways...
| const baseEntityKey = Object.keys(baseEntity)[0]; | ||
| const extractedBaseEntity = baseEntity[baseEntityKey]; | ||
|
|
||
| const combineEntityWithSiblings = (entity: any) => { |
There was a problem hiding this comment.
Is there a reason we need it as any here and can't be a T or an EntityType or something more strongly typed perhaps?
| @@ -2,7 +2,7 @@ import { useAppConfig } from '../useAppConfig'; | |||
|
|
|||
| const useSearchAndBrowseVersion = () => { | |||
| const appConfig = useAppConfig(); | |||
| const searchVersion = appConfig.config.featureFlags.showSearchFiltersV2 ? 'v2' : 'v1'; | |||
| const searchVersion = appConfig.config.featureFlags.showSearchFiltersV2 ? 'v1' : 'v2'; | |||
There was a problem hiding this comment.
Need a revert on this change
There was a problem hiding this comment.
oh goodness
| if (!baseEntity) { | ||
| return baseEntity; | ||
| } | ||
| const baseEntityKey = Object.keys(baseEntity)[0]; |
There was a problem hiding this comment.
@chriscollins3456 just seeing this now. What is the idea behind converting the object to keys and taking whatever the first entry might be?
The bulk of the diff in this PR combines sibling results when showing search result cards - this clears some confusion when a sibling search result sometimes shows all info like dataset stats and other times it doesn't (same idea for tags, glossary terms, etc.).
The other two minor edits are:
Checklist