feat(frontend): show browse paths for business attributes related entities - schema fields #11585

kartikey-visa · 2024-10-10T08:15:40Z

…elds)

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

chriscollins3456

looking solid! i have a few suggestions to make sure things work as expected and to clean it up. nothing crazy then i think it's good to go

chriscollins3456 · 2024-10-24T16:11:22Z

datahub-graphql-core/src/main/resources/entity.graphql

+"""
+Additional properties about EntityName
+"""
+type EntityNameProperties {


so i see you using this as a means to add typing to your frontend properties. however we should only be adding things to the graphql schema that we plan to map back to the client, otherwise it would get confusing what's just existing as a type for the FE vs what exists as a returnable type from GMS

we can always define an interface or type in typescript if we need to cast objects on the frontend into a specific type and I would suggest we do that here instead

chriscollins3456 · 2024-10-24T16:13:03Z

datahub-web-react/src/app/entity/schemaField/SchemaFieldPropertiesEntity.tsx

-    );
+    renderPreview = (previewType: PreviewType, data: SchemaFieldEntity) => {
+        const parent = data.parent as Dataset;
+        const properties = parent.properties as EntityNameProperties;


could we not just use the properties on Dataset here (DatasetProperties)? they already have name and qualifiedName in the correct types i believe right?

DatasetProperties require lastModified as a mandatory field which in our case was missing, hence we defined a new type EntityNameProperties containing only name and qualifiedName.

chriscollins3456 · 2024-10-24T16:19:36Z

datahub-web-react/src/app/preview/DefaultPreviewCard.tsx

@@ -284,6 +289,9 @@ export default function DefaultPreviewCard({
                        parentEntities={parentEntities}
                        parentContainersRef={contentRef}
                        areContainersTruncated={isContentTruncated}
+                        parentDatasetUrn={urn}


i think it might be nice for DefaultPreviewCard to explicitly take in an optional parentDatasetUrn and pass that in from the schema field since right now passing in parentDatasetUrn={urn} would mean every entity would pass in their own urn as the parentDatasetUrn which could definitely cause confusion down the line

chriscollins3456 · 2024-10-24T16:20:34Z

...eact/src/app/entity/shared/containers/profile/header/PlatformContent/PlatformContentView.tsx

+            {parentDatasetUrn && parentDatasetProperties && parentDatasetIcon && (
+                <span>
+                    <StyledRightOutlined data-testid="right-arrow" />
+                    <DatasetLink
+                        parentDatasetUrn={parentDatasetUrn}
+                        parentDatasetProperties={parentDatasetProperties}
+                        parentDatasetIcon={parentDatasetIcon}
+                    />
+                </span>
+            )}


would you mind posting a screenshot of the final output in your PR description so we can see how this change looks in the UI?

chriscollins3456 · 2024-10-24T16:24:37Z

...ub-web-react/src/app/entity/shared/containers/profile/header/PlatformContent/DatasetLink.tsx

+    if (!parentDatasetUrn && !parentDatasetProperties) return null;
+
+    const datasetUrl = entityRegistry.getEntityUrl(EntityType.Dataset, parentDatasetUrn);
+    const datasetName = entityRegistry.getDisplayName(EntityType.Dataset, parentDatasetProperties);


does this retrieve the correct dataset name? i believe entityRegistry.getDisplayName takes in a whole entity object, not just the properties object.

maybe it makes more sense to simply pass along the whole parentDataset object from Preview > DefaultPreviewCard > DatasetLink instead? then you can get the urn, the icon and the whatever else you may need where you need it

Yes, it retrieves the correct dataset name as entityRegistry.getDisplayName function looks for properties.name, if not present it looks for name in the object passed, we were passing the following object {properties : {name: datasetName, qualifiedName: datasetQualifiedName}}, so it was able to extract correct dataset name.

However, as suggested, now we are passing parentDataset object from Preview, which addresses the following things:

passing parentDataset object as Dataset from Preview which includes urn and properties

doesn't require parentDatasetUrn to be passed explicitly

no need to define type/interface for parentDatasetProperties as we did earlier by defining EntityNameProperties

This addresses the above comments as well.

chriscollins3456

nice thank you for addressing those comments! i have one more quick request and that's to remove the parentDatasetIcon prop that's being passed multiple levels down and just get the icon using entity registry in DatasetLink

chriscollins3456 · 2024-10-28T16:07:22Z

...ub-web-react/src/app/entity/shared/containers/profile/header/PlatformContent/DatasetLink.tsx

+
+    return (
+        <StyledLink to={datasetUrl} data-testid="dataset">
+            <DatasetIcon>{parentDatasetIcon}</DatasetIcon>


i think passing parentDatasetIcon down these levels is unnecessary since it should always just be a regular dataset icon you get by using entityRegistry.getIcon(EntityType.Dataset, 14, IconStyleType.ACCENT) we could just get the icon here in this component and if we ever want to start providing dynamic icons we can accept that as a prop for this component

Agreed, I have made the changes suggested above. Thanks!

…ities - schema fields

chriscollins3456

looking good!

…ities - schema fields (datahub-project#11585)

github-actions bot added product PR or Issue related to the DataHub UI/UX community-contribution PR or Issue raised by member(s) of DataHub Community labels Oct 10, 2024

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from d70479c to 61e8a7e Compare October 10, 2024 08:26

vercel bot deployed to Preview October 10, 2024 08:53 View deployment

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from 61e8a7e to 8946e05 Compare October 13, 2024 12:22

vercel bot deployed to Preview October 13, 2024 12:41 View deployment

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from 8946e05 to 7de9982 Compare October 14, 2024 09:50

kartikey-visa changed the title ~~Show browse paths for business attributes related entities (schema fields)~~ feat(frontend): show browse paths for business attributes related entities - schema fields Oct 14, 2024

vercel bot deployed to Preview October 14, 2024 10:08 View deployment

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from 7de9982 to c4723ca Compare October 16, 2024 18:51

vercel bot deployed to Preview October 16, 2024 19:13 View deployment

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from c4723ca to c4904fe Compare October 22, 2024 08:40

vercel bot deployed to Preview October 22, 2024 08:59 View deployment

chriscollins3456 reviewed Oct 24, 2024

View reviewed changes

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from c4904fe to bf2f357 Compare October 27, 2024 17:35

vercel bot deployed to Preview October 27, 2024 17:52 View deployment

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from bf2f357 to 3e3fe70 Compare October 27, 2024 20:06

vercel bot deployed to Preview October 27, 2024 20:22 View deployment

chriscollins3456 reviewed Oct 28, 2024

View reviewed changes

feat(frontend): show browse paths for business attributes related ent…

1fcaa13

…ities - schema fields

kartikey-visa force-pushed the feature/schema-field-browse-paths branch from 3e3fe70 to 1fcaa13 Compare October 28, 2024 17:31

vercel bot deployed to Preview October 28, 2024 18:16 View deployment

chriscollins3456 approved these changes Oct 28, 2024

View reviewed changes

chriscollins3456 merged commit 25a4eef into datahub-project:master Oct 28, 2024
42 checks passed

sleeperdeep pushed a commit to sleeperdeep/datahub that referenced this pull request Dec 17, 2024

feat(frontend): show browse paths for business attributes related ent…

bf40850

…ities - schema fields (datahub-project#11585)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): show browse paths for business attributes related entities - schema fields #11585

feat(frontend): show browse paths for business attributes related entities - schema fields #11585

kartikey-visa commented Oct 10, 2024 •

edited

Loading

chriscollins3456 left a comment

chriscollins3456 Oct 24, 2024

chriscollins3456 Oct 24, 2024

kartikey-visa Oct 27, 2024 •

edited

Loading

chriscollins3456 Oct 24, 2024

chriscollins3456 Oct 24, 2024

kartikey-visa Oct 27, 2024

chriscollins3456 Oct 24, 2024

kartikey-visa Oct 27, 2024 •

edited

Loading

chriscollins3456 left a comment

chriscollins3456 Oct 28, 2024

kartikey-visa Oct 28, 2024

chriscollins3456 left a comment

feat(frontend): show browse paths for business attributes related entities - schema fields #11585

feat(frontend): show browse paths for business attributes related entities - schema fields #11585

Conversation

kartikey-visa commented Oct 10, 2024 • edited Loading

Checklist

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikey-visa Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikey-visa Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

kartikey-visa commented Oct 10, 2024 •

edited

Loading

kartikey-visa Oct 27, 2024 •

edited

Loading

kartikey-visa Oct 27, 2024 •

edited

Loading