Skip to content

Conversation

Ankit-Keshari-Vituity
Copy link
Contributor

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Unit Test Results (build & test)

562 tests   562 ✔️  12m 59s ⏱️
139 suites      0 💤
139 files        0

Results for commit cf3f113.

♻️ This comment has been updated with latest results.

const { data } = useGetTagQuery({ variables: { urn } });
return (
<HoverEntityTooltip>
<TagLink key={data?.tag?.urn}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this should actually be a link to the tag page - so if you look at TagTermGroup.tsx you can export the TagLink component and simply import that here! you'll need to set the link like we do in that component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and committed!!


return (
<HoverEntityTooltip>
<TermLink key={data?.glossaryTerm?.urn}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here - replicate what we do in TagTermGroup.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and committed!!

Comment on lines 258 to 262
const snippet = result.matchedFields[0].value.includes('urn:li:tag') ? (
<TagSummary urn={result.matchedFields[0].value} />
) : (
<TermSummary urn={result.matchedFields[0].value} />
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's guaranteed that if this is not a tag then it is a term (in fact I don't think we surface terms here at all?)

I would say if it includes this urn:li:tag then use the tags summary, then do another check if it contains urn:li:glossaryTerm and use the terms summary, then if neither of those match, use <b>{result.matchedFields[0].value}</b> as it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and committed!!

@jjoyce0510 jjoyce0510 added the product PR or Issue related to the DataHub UI/UX label Sep 9, 2022
@jjoyce0510
Copy link
Collaborator

Build & test is failing!

if (result.matchedFields.length > 0) {
if (result.matchedFields[0].value.includes('urn:li:tag')) {
snippet = <TagSummary urn={result.matchedFields[0].value} />;
} else if (result.matchedFields[0].value.includes('urn:li:term')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs to be urn:li:glossaryTerm

<TermLink key={data?.glossaryTerm?.urn}>
<Tag closable={false} style={{ cursor: 'pointer' }}>
<BookOutlined style={{ marginRight: '3%' }} />
{data?.glossaryTerm?.name}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you use entityRegistry.getDisplayName here?

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

nice!

@chriscollins3456 chriscollins3456 merged commit b4ddada into datahub-project:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants