-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Combine siblings in autocomplete #8610
Changes from 19 commits
0975526
60eee46
0116d15
4aa73e1
844552a
e72aaea
92dd8db
5f85017
91649e2
6953c63
f6571f0
7707b54
931570a
5b2395c
4d573d7
45b8ded
fe9c179
e89afc2
5620c5e
97e2b84
7c8d0b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import { Input, AutoComplete, Button } from 'antd'; | |
import { CloseCircleFilled, SearchOutlined } from '@ant-design/icons'; | ||
import styled from 'styled-components/macro'; | ||
import { useHistory } from 'react-router'; | ||
import { AutoCompleteResultForEntity, Entity, EntityType, FacetFilterInput, ScenarioType } from '../../types.generated'; | ||
import { AutoCompleteResultForEntity, EntityType, FacetFilterInput, ScenarioType } from '../../types.generated'; | ||
import EntityRegistry from '../entity/EntityRegistry'; | ||
import filterSearchQuery from './utils/filterSearchQuery'; | ||
import { ANTD_GRAY, ANTD_GRAY_V2 } from '../entity/shared/constants'; | ||
|
@@ -23,6 +23,7 @@ import { navigateToSearchUrl } from './utils/navigateToSearchUrl'; | |
import { getQuickFilterDetails } from './autoComplete/quickFilters/utils'; | ||
import ViewAllSearchItem from './ViewAllSearchItem'; | ||
import { ViewSelect } from '../entity/view/select/ViewSelect'; | ||
import { combineSiblingsInAutoComplete } from './utils/combineSiblingsInAutoComplete'; | ||
|
||
const StyledAutoComplete = styled(AutoComplete)` | ||
width: 100%; | ||
|
@@ -88,15 +89,6 @@ const QUICK_FILTER_AUTO_COMPLETE_OPTION = { | |
], | ||
}; | ||
|
||
const renderItem = (query: string, entity: Entity) => { | ||
return { | ||
value: entity.urn, | ||
label: <AutoCompleteItem query={query} entity={entity} />, | ||
type: entity.type, | ||
style: { padding: '12px 12px 12px 16px' }, | ||
}; | ||
}; | ||
|
||
const renderRecommendedQuery = (query: string) => { | ||
return { | ||
value: query, | ||
|
@@ -123,6 +115,7 @@ interface Props { | |
hideRecommendations?: boolean; | ||
showQuickFilters?: boolean; | ||
viewsEnabled?: boolean; | ||
combineSiblings?: boolean; | ||
setIsSearchBarFocused?: (isSearchBarFocused: boolean) => void; | ||
onFocus?: () => void; | ||
onBlur?: () => void; | ||
|
@@ -149,6 +142,7 @@ export const SearchBar = ({ | |
hideRecommendations, | ||
showQuickFilters, | ||
viewsEnabled = false, | ||
combineSiblings = false, | ||
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. so we default want to not combine siblings in our search bar? is there somewhere where we particularly don't want them combined? 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. 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. |
||
setIsSearchBarFocused, | ||
onFocus, | ||
onBlur, | ||
|
@@ -227,14 +221,26 @@ export const SearchBar = ({ | |
]; | ||
}, [showQuickFilters, suggestions.length, effectiveQuery, selectedQuickFilter, entityRegistry]); | ||
|
||
const autoCompleteEntityOptions = useMemo( | ||
() => | ||
suggestions.map((entity: AutoCompleteResultForEntity) => ({ | ||
label: <SectionHeader entityType={entity.type} />, | ||
options: [...entity.entities.map((e: Entity) => renderItem(effectiveQuery, e))], | ||
})), | ||
[effectiveQuery, suggestions], | ||
); | ||
const autoCompleteEntityOptions = useMemo(() => { | ||
return suggestions.map((suggestion: AutoCompleteResultForEntity) => { | ||
const combinedSuggestion = combineSiblingsInAutoComplete(suggestion, { combineSiblings }); | ||
return { | ||
label: <SectionHeader entityType={combinedSuggestion.type} />, | ||
options: combinedSuggestion.combinedEntities.map((combinedEntity) => ({ | ||
value: combinedEntity.entity.urn, | ||
label: ( | ||
<AutoCompleteItem | ||
query={effectiveQuery} | ||
entity={combinedEntity.entity} | ||
siblings={combineSiblings ? combinedEntity.matchedEntities : undefined} | ||
/> | ||
), | ||
type: combinedEntity.entity.type, | ||
style: { padding: '12px 12px 12px 16px' }, | ||
})), | ||
}; | ||
}); | ||
}, [combineSiblings, effectiveQuery, suggestions]); | ||
|
||
const previousSelectedQuickFilterValue = usePrevious(selectedQuickFilter?.value); | ||
useEffect(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,15 @@ | ||
import { Image, Typography } from 'antd'; | ||
import { Typography } from 'antd'; | ||
import React from 'react'; | ||
import styled from 'styled-components/macro'; | ||
import { Entity } from '../../../types.generated'; | ||
import { useEntityRegistry } from '../../useEntityRegistry'; | ||
import { getPlatformName } from '../../entity/shared/utils'; | ||
import { IconStyleType } from '../../entity/Entity'; | ||
import { getAutoCompleteEntityText } from './utils'; | ||
import { SuggestionText } from './AutoCompleteUser'; | ||
import ParentContainers from './ParentContainers'; | ||
import { ANTD_GRAY } from '../../entity/shared/constants'; | ||
import { ANTD_GRAY_V2 } from '../../entity/shared/constants'; | ||
import AutoCompleteEntityIcon from './AutoCompleteEntityIcon'; | ||
import { SuggestionText } from './styledComponents'; | ||
import AutoCompletePlatformNames from './AutoCompletePlatformNames'; | ||
import { capitalizeFirstLetterOnly } from '../../shared/textUtil'; | ||
|
||
const AutoCompleteEntityWrapper = styled.div` | ||
display: flex; | ||
|
@@ -17,12 +18,8 @@ const AutoCompleteEntityWrapper = styled.div` | |
align-items: center; | ||
`; | ||
|
||
const PreviewImage = styled(Image)` | ||
height: 22px; | ||
width: 22px; | ||
width: auto; | ||
object-fit: contain; | ||
background-color: transparent; | ||
const IconsContainer = styled.div` | ||
display: flex; | ||
`; | ||
|
||
const ContentWrapper = styled.div` | ||
|
@@ -32,42 +29,82 @@ const ContentWrapper = styled.div` | |
`; | ||
|
||
const Subtype = styled.span` | ||
color: ${ANTD_GRAY[9]}; | ||
border: 1px solid ${ANTD_GRAY[9]}; | ||
color: ${ANTD_GRAY_V2[8]}; | ||
border: 1px solid ${ANTD_GRAY_V2[6]}; | ||
border-radius: 16px; | ||
padding: 4px 8px; | ||
line-height: 12px; | ||
font-size: 12px; | ||
margin-right: 8px; | ||
`; | ||
|
||
const ItemHeader = styled.div` | ||
display: flex; | ||
align-items: center; | ||
margin-bottom: 3px; | ||
gap: 8px; | ||
`; | ||
|
||
const Divider = styled.div` | ||
border-right: 1px solid ${ANTD_GRAY_V2[6]}; | ||
height: 12px; | ||
`; | ||
|
||
interface Props { | ||
query: string; | ||
entity: Entity; | ||
siblings?: Array<Entity>; | ||
hasParentTooltip: boolean; | ||
} | ||
|
||
export default function AutoCompleteEntity({ query, entity, hasParentTooltip }: Props) { | ||
export default function AutoCompleteEntity({ query, entity, siblings, hasParentTooltip }: Props) { | ||
const entityRegistry = useEntityRegistry(); | ||
const genericEntityProps = entityRegistry.getGenericEntityProperties(entity.type, entity); | ||
const platformName = getPlatformName(genericEntityProps); | ||
const platformLogoUrl = genericEntityProps?.platform?.properties?.logoUrl; | ||
const displayName = entityRegistry.getDisplayName(entity.type, entity); | ||
const icon = | ||
(platformLogoUrl && <PreviewImage preview={false} src={platformLogoUrl} alt={platformName || ''} />) || | ||
entityRegistry.getIcon(entity.type, 12, IconStyleType.ACCENT); | ||
const { matchedText, unmatchedText } = getAutoCompleteEntityText(displayName, query); | ||
const parentContainers = genericEntityProps?.parentContainers?.containers || []; | ||
// Need to reverse parentContainers since it returns direct parent first. | ||
const orderedParentContainers = [...parentContainers].reverse(); | ||
const subtype = genericEntityProps?.subTypes?.typeNames?.[0]; | ||
const entities = siblings?.length ? siblings : [entity]; | ||
|
||
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); | ||
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. nit: i feel like this could be pulled out into its own function. Also I see elsewhere that we have 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. Updated! I left the iteration in this component but reused an existing helper function we already have. |
||
|
||
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) ?? []; | ||
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 feel like this should be taken care of when we combine sibling data so we end up just getting one In fact, with my changes in #8602 we can now get proper 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. Awesome. So like if one sibling had no parentContainers and another one missed them, it would automatically choose the one that had them present? 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. It does seem to work thought! Shows the bigquery containers even on the dbt primary. 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. 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 |
||
|
||
const showPlatforms = !!platformNames.length; | ||
const showPlatformDivider = !!platformNames.length && !!parentContainers.length; | ||
const showParentContainers = !!parentContainers.length; | ||
const showHeader = showPlatforms || showParentContainers; | ||
|
||
return ( | ||
<AutoCompleteEntityWrapper data-testid={`auto-complete-entity-name-${displayName}`}> | ||
<ContentWrapper> | ||
{icon} | ||
<SuggestionText> | ||
<ParentContainers parentContainers={orderedParentContainers} /> | ||
{showHeader && ( | ||
<ItemHeader> | ||
<IconsContainer> | ||
{entities.map((ent) => ( | ||
<AutoCompleteEntityIcon key={ent.urn} entity={ent} /> | ||
))} | ||
</IconsContainer> | ||
{showPlatforms && <AutoCompletePlatformNames platforms={platformNames} />} | ||
{showPlatformDivider && <Divider />} | ||
{showParentContainers && <ParentContainers parentContainers={parentContainers} />} | ||
</ItemHeader> | ||
)} | ||
<Typography.Text | ||
ellipsis={ | ||
hasParentTooltip ? {} : { tooltip: { title: displayName, color: 'rgba(0, 0, 0, 0.9)' } } | ||
|
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.
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 usesThere 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.
Think I got the conflict resolved 👍