-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(ui) Add new embedded profile to be displayed in extension #7113
Conversation
…skeleton-and-header feat(ui) Build skeleton + header of new embedded profile
…t-section feat(ui) Update SidebarAboutSection and use in EmbeddedProfile
…sidebar-sections feat(ui) Make Owner, Tag, Term, Domain sections optionally readOnly
…ealth feat(ui) Add upstream health component to embedded profile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Reviewed offline :)
Seems that some build is failing :) |
Minor copy comments added offline |
pictureLink?: string; | ||
} | ||
|
||
export default function OwnerContent({ name, owner, hidePopOver, pictureLink }: Props) { |
There was a problem hiding this comment.
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 } })}> |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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>) { |
There was a problem hiding this comment.
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 && ( | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm yes please
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
}); | ||
}; | ||
|
||
return ( |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this 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.
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:
When there is at least one passing upstream assertion and none failing:
When there are no assertions upstream or no upstreams at all:
Checklist