feat(frontend): add additional tabs to glossary terms view #7392
Conversation
add "contained by" and "inherited by" tabs to glossary related terms view
anshbansal
added
the
community-contribution
There was a problem hiding this comment.
This is great stuff @alexey-kravtsov ! thanks so much for this contribution.
I only have a few comments and all of the are real small changes for consistency and grammar. Once you get those up I believe we'll be ready to merge!
| @@ -12,6 +12,8 @@ import RelatedTerm from './RelatedTerm'; | |||
| export enum RelatedTermTypes { | |||
| hasRelatedTerms = 'Contains', | |||
| isRelatedTerms = 'Inherits', | |||
| containedBy = 'Contained by', | |||
| isAChildren = 'Inherited by', | |||
There was a problem hiding this comment.
just to be a bit more consistent with containedBy what do you think about calling this enum entry inheritedBy?
There was a problem hiding this comment.
actually keys of this enum are implicitly used to address entityData fields here https://github.com/datahub-project/datahub/blob/master/datahub-web-react/src/app/entity/glossaryTerm/profile/GlossaryRelatedTerms.tsx#L61 , and isAChildren is a field of type GenericEntityProperties. I can change this enum to structure like this
export const RELATED_TERM_TYPES = {
inheritedBy: {
title: 'Inherited by',
fieldName: 'isAChildren',
},
...
How do you think, would that be better? Or maybe just leave it as is?
There was a problem hiding this comment.
hey great call! I think we can leave it as is in that case.
I wouldn't be opposed to updating glossaryTerm.graphql and changing isAChildren -> inheritedBy and then updating GenericEntityProperties and anywhere that references the field isAChildren, however I think that's out of scope here and i'm cool with moving forward with what you have.
| ? TermRelationshipType.HasA | ||
| : TermRelationshipType.IsA; | ||
| const relatedTermsMutable = |
There was a problem hiding this comment.
maybe a rename to canEditRelatedTerms?
| key={urn} | ||
| urn={urn} | ||
| relationshipType={relationshipType} | ||
| mutable={relatedTermsMutable} |
There was a problem hiding this comment.
and similarly we could change this prop from mutable to isEditable (just cuz we usually use the verb "edit" instead of "mutate" for this type of thing and prefer descriptive booleans like isEditable instead of just editable)
| @@ -57,6 +57,14 @@ export const EMPTY_MESSAGES = { | |||
| title: 'Does not inherit from any terms', | |||
| description: 'Terms can inherit from other terms to represent an "Is A" style relationship.', | |||
| }, | |||
| 'contained by': { | |||
| title: 'Does not contained by any terms', | |||
There was a problem hiding this comment.
copy change: 'Does not contained by any terms' -> 'Is not contained by any terms'
| @@ -57,6 +57,14 @@ export const EMPTY_MESSAGES = { | |||
| title: 'Does not inherit from any terms', | |||
| description: 'Terms can inherit from other terms to represent an "Is A" style relationship.', | |||
| }, | |||
| 'contained by': { | |||
| title: 'Does not contained by any terms', | |||
| description: 'Terms can be contained by other terms to represent an "Has A" style relationship.', | |||
There was a problem hiding this comment.
we can change the ...respresent an "Has A"... to ...represent a "Has A"...
also if you wouldn't mind making that "an" -> "a" change on line 54 as well that would be much appreciated :)
| description: 'Terms can be contained by other terms to represent an "Has A" style relationship.', | ||
| }, | ||
| 'inherited by': { | ||
| title: 'Does not inherited by any terms', |
There was a problem hiding this comment.
Similar to above -> Is not inherited by an terms
changed variable name and tab labels according to PR comments
|
Thanks a lot @chriscollins3456 for your review! I addressed all comments except one |
| description: 'Terms can be contained by other terms to represent a "Has A" style relationship.', | ||
| }, | ||
| 'inherited by': { | ||
| title: 'Is not inherited by an terms', |
There was a problem hiding this comment.
shoot - you might have gotten this from my typo from a comment on this line, can we change "an" -> "any" here so it would be:
title: 'Is not inherited by any terms',
fix title typo
…roject#7392) Adds tabs for "Contained by" and "Inherited by" on the Glossary Related Terms tab.
Adds tabs for "Contained by" and "Inherited by" on the Glossary Related Terms tab.
Add "Contained by" and "Inherited by" tabs to glossary related terms view as requested in feature request https://feature-requests.datahubproject.io/p/list-inbound-relationships-to-glossary-terms
Checklist