feat(React): Ownership component of user profile #2173

brendansun93 · 2021-03-04T19:56:40Z

Scope
This PR only changes User profile component of React-web.

Changes
This PR adds existing UserOwnership component to UserDetail component and adds data to display coming from GraphQL search query.

Below screenshot is taken from visiting http://localhost:3000/user/urn:li:corpuser:datahub/ownership/dataset

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)

brendansun93 · 2021-03-04T19:58:32Z

datahub-web-react/src/app/entity/user/UserProfile.tsx

+
+    const ownershipResult: any = {};
+
+    ownershipResult[EntityType.Chart] = useGetSearchResultsQuery({


Since hook cannot be called inside any loop, not sure if there is a way in Typescript that can make it dynamic, i.e. looping through all enum values and get search query

We'd like to add support for an all endpoint in the future. In the meantime, I'd recommend encapsulating this all in a 2nd hook useGetEachEntitySearchResults so that the UserProfile render function isn't so large

+1 to wrapping this

brendansun93 · 2021-03-04T19:59:05Z

@jjoyce0510 @gabe-lyons @shirshanka Please review

gabe-lyons

Nice! Looks good, although i do think it would benefit from encapsulating the multiplex search into its own helper util

gabe-lyons · 2021-03-04T21:06:07Z

@brendansun I also noticed that the preview card looks squashed in your screenshot, do you know why that is? Does it looks the same as the dataset preview in search?

brendansun93 · 2021-03-04T21:08:14Z

@brendansun I also noticed that the preview card looks squashed in your screenshot, do you know why that is? Does it looks the same as the dataset preview in search?

No, the search result page shows ok. I think it is because the left side menu?

gabe-lyons · 2021-03-04T21:28:18Z

@brendansun I see- feel free to change the UserOwnership component as you need to make them look the same

jjoyce0510 · 2021-03-04T22:12:08Z

It looks like there should be more padding / margin on the individual list items that are rendered within the ownership details component... Can we include in this PR?

brendansun93 · 2021-03-04T22:18:35Z

It looks like there should be more padding / margin on the individual list items that are rendered within the ownership details component... Can we include in this PR?

OK got it, I can do that, will let you guys know when I am done with it

brendansun93 · 2021-03-05T20:27:59Z

@gabe-lyons @jjoyce0510 I have made requested changes, please take a look again; also, the screenshot is updated with some vertical margin to look better 😄

gabe-lyons · 2021-03-06T01:29:35Z

@brendansun looks better- can you apply some extra padding on the first element so it is spaced out from the header equally to the element below it?

jjoyce0510 · 2021-03-08T11:51:44Z

datahub-web-react/src/app/entity/user/UserOwnership.tsx

-                    return entityRegistry.renderPreview(entityType, PreviewType.PREVIEW, item);
+                    return (
+                        <div
+                            style={{


Nit: can we extract out styles into a const variable at the top of the file?

Removed inline style

jjoyce0510 · 2021-03-08T11:52:29Z

datahub-web-react/src/utils/customGraphQL/useGetEachEntitySearchResults.ts

+import { EntityType } from '../../types.generated';
+import { useGetSearchResultsQuery } from '../../graphql/search.generated';
+
+export function useGetEachEntitySearchResults(input: any) {


nice!

very minor nit:

can we call this "useGetAllEntitySearchResults"

jjoyce0510 · 2021-03-08T11:53:28Z

datahub-web-react/src/utils/customGraphQL/useGetEachEntitySearchResults.ts

+    result[EntityType.Chart] = useGetSearchResultsQuery({
+        variables: {
+            input: {
+                type: EntityType.Chart,


can we pass a "start" and "count" variable into this method and add it here? That way we can control how many results we get for each entity easily. I will likely replace my current entity loading in the "all" tab with what you've added here :)

This is already handled by the input object passed in to keep it generic, i.e. if we pass an object with start and count, then this query will get it. Do we want to explicitly mark these 2 parameters?

Please see latest change, I have implemented the input using Partial<SearchInput> so that start and count are checked when compile

Thank you! Looks great.

brendansun93 · 2021-03-08T15:26:31Z

@brendansun looks better- can you apply some extra padding on the first element so it is spaced out from the header equally to the element below it?

I have changed the code of this page a bit because Ant component doesnt allow us to style the margin for first item or the entire ul html element. Therefore I moved the Title out of list header with a divider under it now. Please see the screenshot i have just uploaded to description. If there should be more margin, please let me know and it will be a easy fix :)

gabe-lyons · 2021-03-08T15:46:41Z

@brendansun looks good to me 🚀

gabe-lyons · 2021-03-08T15:48:13Z

datahub-web-react/src/utils/customGraphQL/useGetAllEntitySearchResults.ts

+import { EntityType } from '../../types.generated';
+import { useGetSearchResultsQuery } from '../../graphql/search.generated';
+
+export function useGetAllEntitySearchResults(input: any) {


we should be able to get this type from types.generated- you don't need to leave this as an any

Specifically it should be SearchInput from src/types.generated.ts

If you want to allow any subset of fields from the type you could consider using Partial<SearchInput> as the type

Thanks for this information about Partial

gabe-lyons · 2021-03-08T15:50:03Z

datahub-frontend/run/frontend.env

@@ -1,5 +1,5 @@
 # Server Port
-PORT=9001
+PORT=9002


make sure to change this back to 9001 before merging!

Reverted, thanks for the reminder

shirshanka

LGTM! Thanks @brendansun93 for the contrib and @gabe-lyons for the review!

jjoyce0510

LGTM. Thank you so much!

jjoyce0510 · 2021-03-09T19:24:46Z

datahub-web-react/src/utils/customGraphQL/useGetAllEntitySearchResults.ts

+
+type AllEntityInput<T, K> = Pick<T, Exclude<keyof T, keyof K>> & K;
+
+export function useGetAllEntitySearchResults(input: AllEntityInput<SearchInput, { type?: EntityType }>) {


Hey just realized we should use the Entity Registry here. Will raise another PR...

brendansun added 3 commits March 4, 2021 14:51

Add ownership component in user profile page

5988f23

revert frontend.env

9bda7c9

cleanup

4330e71

brendansun93 commented Mar 4, 2021

View reviewed changes

gabe-lyons reviewed Mar 4, 2021

View reviewed changes

shirshanka added the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Mar 5, 2021

Increase margin, encapsulate search query

82190f5

jjoyce0510 reviewed Mar 8, 2021

View reviewed changes

Re-styled List; rename method

514ce36

gabe-lyons reviewed Mar 8, 2021

View reviewed changes

More strict type + cleanup

2541942

gabe-lyons approved these changes Mar 8, 2021

View reviewed changes

shirshanka approved these changes Mar 8, 2021

View reviewed changes

shirshanka merged commit e3ad0ed into datahub-project:master Mar 8, 2021

jjoyce0510 reviewed Mar 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(React): Ownership component of user profile #2173

feat(React): Ownership component of user profile #2173

brendansun93 commented Mar 4, 2021 •

edited

Loading

brendansun93 Mar 4, 2021

gabe-lyons Mar 4, 2021

jjoyce0510 Mar 4, 2021

brendansun93 commented Mar 4, 2021

gabe-lyons left a comment

gabe-lyons commented Mar 4, 2021

brendansun93 commented Mar 4, 2021

gabe-lyons commented Mar 4, 2021

jjoyce0510 commented Mar 4, 2021

brendansun93 commented Mar 4, 2021

brendansun93 commented Mar 5, 2021

gabe-lyons commented Mar 6, 2021

jjoyce0510 Mar 8, 2021

brendansun93 Mar 8, 2021

jjoyce0510 Mar 8, 2021

brendansun93 Mar 8, 2021

jjoyce0510 Mar 8, 2021

brendansun93 Mar 8, 2021

brendansun93 Mar 8, 2021

jjoyce0510 Mar 9, 2021

brendansun93 commented Mar 8, 2021

gabe-lyons commented Mar 8, 2021

gabe-lyons Mar 8, 2021 •

edited

Loading

gabe-lyons Mar 8, 2021

gabe-lyons Mar 8, 2021

brendansun93 Mar 8, 2021

brendansun93 Mar 8, 2021

gabe-lyons Mar 8, 2021

brendansun93 Mar 8, 2021

shirshanka left a comment

jjoyce0510 left a comment

jjoyce0510 Mar 9, 2021


		const ownershipResult: any = {};

		ownershipResult[EntityType.Chart] = useGetSearchResultsQuery({


		type AllEntityInput<T, K> = Pick<T, Exclude<keyof T, keyof K>> & K;

		export function useGetAllEntitySearchResults(input: AllEntityInput<SearchInput, { type?: EntityType }>) {

feat(React): Ownership component of user profile #2173

feat(React): Ownership component of user profile #2173

Conversation

brendansun93 commented Mar 4, 2021 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendansun93 commented Mar 4, 2021

gabe-lyons left a comment

Choose a reason for hiding this comment

gabe-lyons commented Mar 4, 2021

brendansun93 commented Mar 4, 2021

gabe-lyons commented Mar 4, 2021

jjoyce0510 commented Mar 4, 2021

brendansun93 commented Mar 4, 2021

brendansun93 commented Mar 5, 2021

gabe-lyons commented Mar 6, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendansun93 commented Mar 8, 2021

gabe-lyons commented Mar 8, 2021

gabe-lyons Mar 8, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirshanka left a comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendansun93 commented Mar 4, 2021 •

edited

Loading

gabe-lyons Mar 8, 2021 •

edited

Loading