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.

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

feat(ui) Add new embedded profile to be displayed in extension #7113

Conversation

chriscollins3456
Copy link
Collaborator

This PR adds a new EmbeddedProfile that will be displayed in an iFrame of our new chrome extension. This profile reuses lots of components from an entity profile sidebar and updates those components to allow them to be readOnly (this refactoring is the cause for a good chunk of the diff). This also creates a new UpstreamHealth component that fetches all upstreams and their assertions so that we can look through and see if anything is failing.

When there are failing upstream assertions:
image

When there is at least one passing upstream assertion and none failing:
image

When there are no assertions upstream or no upstreams at all:
image

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 Jan 23, 2023
Copy link
Contributor

@aditya-radhakrishnan aditya-radhakrishnan left a comment

Choose a reason for hiding this comment

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

LGTM! Reviewed offline :)

@jjoyce0510
Copy link
Collaborator

Seems that some build is failing :)

@jjoyce0510
Copy link
Collaborator

Minor copy comments added offline

pictureLink?: string;
}

export default function OwnerContent({ name, owner, hidePopOver, pictureLink }: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice refactor!

{EMPTY_MESSAGES.documentation.title}. {EMPTY_MESSAGES.documentation.description}
</EmptyContentMessage>
{!readOnly && (
<Button onClick={() => routeToTab({ tabName: 'Documentation', tabParams: { editing: true } })}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

readOnly?: boolean;
}

export default function LinksSection({ hideLinksButton, readOnly }: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just a file being moved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup! just pulled it out from the old bigger SidebarAboutSection component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

link: InstitutionalMemoryMetadata;
}

export default function LinkButton({ link }: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesomee!!!

getOverrideProperties: (T) => GenericEntityProperties;
}

export default function useGetDataForProfile<T>({ urn, entityType, useEntityQuery, getOverrideProperties }: Props<T>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh thank GOD!

</LoadingWrapper>
)}
{!loading && entityData && (
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhm yes please

Copy link
Collaborator

Choose a reason for hiding this comment

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

Look at this clean stack

});
};

const removeTerm = (termToRemove: GlossaryTermAssociation) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How one thing I think almost 100% we will face merge back issues due to proposals. Just calling out!

refetch?: () => Promise<any>;
}

export default function StyledTerm(props: Props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

});
};

return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wowwww!

@@ -781,6 +781,17 @@ fragment searchAcrossRelationshipResults on SearchAcrossLineageResults {
searchResults {
entity {
...searchResultFields
... on Dataset {
assertions @include(if: $includeAssertions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for including this conditionally

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pretty awesome that we can do this btw!

Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

LGTM. I want to be careful about the merge-back process, however, given the scope of the changes.

@chriscollins3456 chriscollins3456 merged commit 1e206c2 into datahub-project:master Jan 26, 2023
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