Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding Domain to Glossary Terms #6135

Merged
merged 11 commits into from
Oct 24, 2022
Merged

Adding Domain to Glossary Terms #6135

merged 11 commits into from
Oct 24, 2022

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 github-actions bot added the product PR or Issue related to the DataHub UI/UX label Oct 6, 2022
@github-actions
Copy link

github-actions bot commented Oct 6, 2022 •
edited
Loading

Unit Test Results (build & test)

597 tests  ±0   593 ✔️ ±0   12m 8s ⏱️ +25s
147 suites ±0       4 💤 ±0 
147 files   ±0       0 ❌ ±0 

Results for commit baed23e. ± Comparison against base commit 0b60225.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Oct 11, 2022 •
edited
Loading

Unit Test Results (metadata ingestion)

       8 files         8 suites   57m 24s ⏱️
   747 tests    744 ✔️ 3 💤 0 ❌
1 496 runs  1 490 ✔️ 6 💤 0 ❌

Results for commit baed23e.

♻️ This comment has been updated with latest results.

chriscollins3456
chriscollins3456 reviewed Oct 11, 2022
View reviewed changes
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.

things are looking good! the last thing you'll need is to query for the domain on the frontend. So in src/graphql/glossaryTerm under the getGlossaryTerm query, you'll need to get:

        domain {
            ...entityDomain
        }

chriscollins3456
chriscollins3456 reviewed Oct 11, 2022
View reviewed changes
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Another thing that needs to be done is that we need to surface the domains of glossary terms on their search preview cards (check below for on the domain page and on the search page where Term 1 is associated with the Marketing domain but you can't see it on the search card)
image

image

chriscollins3456
chriscollins3456 reviewed Oct 13, 2022
View reviewed changes
datahub-web-react/src/app/entity/glossaryTerm/preview/Preview.tsx Outdated
@@ -35,6 +37,7 @@ export const Preview = ({
typeIcon={entityRegistry.getIcon(EntityType.GlossaryTerm, 14, IconStyleType.ACCENT)}
deprecation={deprecation}
parentNodes={parentNodes}
glossaryAssociatedDomain={domain}
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 just reuse the existing domain prop on DefaultPreviewCard instead of creating a new glossaryAssociatedDomain prop?

chriscollins3456
chriscollins3456 approved these changes Oct 24, 2022
View reviewed changes
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 ac67799 into datahub-project:master Oct 24, 2022
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
@Ankit-Keshari-Vituity @cccs-tom
Add the ability to add Domain to Glossary Terms (datahub-project#6135)
dfc4b02
cccs-tom pushed a commit to CybercentreCanada/datahub that referenced this pull request Nov 18, 2022
@Ankit-Keshari-Vituity @cccs-tom
Add the ability to add Domain to Glossary Terms (datahub-project#6135)
7a3a523
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriscollins3456 chriscollins3456 chriscollins3456 approved these changes

Assignees
No one assigned
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ankit-Keshari-Vituity @chriscollins3456