highlight matched fields in search results #8651
Conversation
…don't have an enttiy
| @@ -0,0 +1,40 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
This file is super confusing. I just ported it from the snippet component that was here.
There was a problem hiding this comment.
this is looking real solid! i love how much cleaner matched code is now and love the new context.
I have a few minor suggestions and one question/concern about highlighting text without any matching substrings and a situation that it looks off in search results
datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/types/mappers/MapperUtils.java
Show resolved
Hide resolved
| @@ -665,6 +665,11 @@ type MatchedField { | |||
| Value of the field that matched | |||
| """ | |||
| value: String! | |||
|
|
|||
| """ | |||
| Entity if the value was an urn | |||
| matchedFields={result.matchedFields} | ||
| inputFields={data.inputFields} | ||
| <MatchedFieldList | ||
| customFieldRenderer={(matchedField) => chartMatchedFieldRenderer(matchedField, data)} |
There was a problem hiding this comment.
since this is being used for chats/dashboards maybe a different name could be used? something like matchedInputFieldRenderer since these two entities has inputFields?
There was a problem hiding this comment.
Yeah, sounds good. I'll adjust the dataset one as well.
| return matchedField ? ( | ||
| <Typography.Text> | ||
| Matches {FIELDS_TO_HIGHLIGHT.get(matchedField.name)} <b>{matchedField.value}</b>{' '} | ||
| {isMatchingDashboard && 'on a contained Chart'} |
There was a problem hiding this comment.
do we want to drop the "on a contained Chart" piece?
There was a problem hiding this comment.
Good catch. I'll add a prop for that.
| if (customRenderedField) return <b>{customRenderedField}</b>; | ||
| if (field.value.includes('urn:li:tag') && !field.entity) return <></>; | ||
| if (field.entity) return <>{entityRegistry.getDisplayName(field.entity.type, field.entity)}</>; | ||
| if (field.name.toLowerCase().includes('description') && query) { |
There was a problem hiding this comment.
nit: I think there's at least one or two other places where you're checking if something includes "description" -> could be a constant!
| export const SearchResultProvider = ({ children, searchResult }: Props) => { | ||
| const value = useMemo( | ||
| () => ({ | ||
| searchResult, | ||
| }), | ||
| [searchResult], | ||
| ); | ||
| return <SearchResultContext.Provider value={value}>{children}</SearchResultContext.Provider>; | ||
| }; |
| const customRenderedField = customFieldRenderer?.(field); | ||
| if (customRenderedField) return <b>{customRenderedField}</b>; | ||
| if (field.value.includes('urn:li:tag') && !field.entity) return <></>; | ||
| if (field.entity) return <>{entityRegistry.getDisplayName(field.entity.type, field.entity)}</>; |
There was a problem hiding this comment.
do you know all the types of entities that could be in this field? obviously tag and glossary term and the first that come to mind. Just want to make sure we don't get to a situation where we pass in an entity not configured in our frontend EntityRegistry that causes an error to be thrown and a white screen of death
| const hasSubstring = hasMatchedField && !!normalizedSearchQuery && normalizedText.includes(normalizedSearchQuery); | ||
| const pattern = enableFullHighlight ? HIGHLIGHT_ALL_PATTERN : undefined; |
There was a problem hiding this comment.
i'm a little confused by the full highlight situation - why do we want to highlight the full text if it doesn't match anything in the search query? after pulling down I'm seeing situations like this where my search query is long_tail and ADOPTIONS is being highlighted (but I believe these results are showing up because long_tail matches their container's name)
There was a problem hiding this comment.
I don't seem to have this same data to repro this, will ping ya.
There was a problem hiding this comment.
The main reason for highlighting all is for situation where you search for baz 2 we would still end up highlighting 'Baz Chart 2' since it has a title match. Otherwise, we'll end up not highlighting it. The idea here was to trust the backend, if it's saying we matched the name, let's highlight the name to indicate as such.
There was a problem hiding this comment.
Removed qualifiedName from fields we highlight since we only use that for container names and the solve would be complex.
There was a problem hiding this comment.
nice this solved my issue i was seeing
| entity { | ||
| urn | ||
| type | ||
| ...entityDisplayNameFields | ||
| } |
| """ | ||
| Whether a search result should highlight the name/description if it was matched on those fields. | ||
| """ | ||
| enableNameHighlight: Boolean |
There was a problem hiding this comment.
thank you for adding this!
Substantial overhaul to the visual search highlighting approach in datahub.
Changes