Added a external link to noobaa details card #2520

bipuladh · 2019-08-28T11:17:30Z

As shown in the screenshot below a external link has been added under System name which redirects the user to an external link.

openshift-ci-robot · 2019-08-28T11:17:44Z

Hi @bipuladh. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

afreen23 · 2019-08-28T11:26:40Z

frontend/packages/noobaa-storage-plugin/src/components/details-card/details-card.tsx

we need to handle the case when the systemName and systemAddress will be not defined and hence systemLink will be broken.
can refer https://github.com/openshift/console/blob/master/frontend/packages/noobaa-storage-plugin/src/components/buckets-card/buckets-card.tsx#L70

The systemError variable below should handle such case, unless I'm missing something.

The systemLink variable could be defined as

const systemLink = !systemError ? `${systemAddress}fe/systems/${systemName}` : undefined;

to make it clear that systemLink as a whole is undefined unless both systemName and systemAddress are valid.

Anyway, I'd suggest to simplify it further, just use

const systemLink = (systemName && systemAddress) ? `${systemAddress}fe/systems/${systemName}` : undefined;

and in JSX code below

error={!systemLink}

afreen23 · 2019-08-28T11:32:32Z

frontend/public/components/dashboard/details-card/detail-item.tsx

@cloudbehl @rawagner @gnehapk perceptions on this custom details link ?

nice, i'd like to use the same approach as we do for InventoryItem where we have a generic component, which takes isLoading, error and children https://github.com/openshift/console/blob/master/frontend/public/components/dashboard/inventory-card/inventory-item.tsx#L47.

In this case the child would be either string or ExternalLink or any other component if we need in the future. If error is true, then the unavailable is shown etc.

the basic component would be just Item and we will have DetailItem and DetailItemExternalLink

Thanks @rawagner for pointing that out. However I will not be adding any required types to it and make error state detectable from the current parameters and also from the error parameter(optional) so that these changes do not break any existing references(However there are not many references and making changes to them is possible allowing us to make error a required parameter). Following your approach I don't think we will need to create a new component DetailItemExternalLink since we will be passing ExternalLink or other similar components as children.

alright @rawagner , not aware of the implementation in InventoryItem.Thank you!
Late to comment but I just did not find it appropriate and rather expecting something like that the ExternalLink to be passed in value.

bipuladh · 2019-08-28T11:32:54Z

frontend/public/components/dashboard/details-card/detail-item.tsx

systemName being tested to see if it is undefined. Since systemName and systemAddress are queried from the same object, at the same depth. @afreen23

bipuladh · 2019-08-29T13:50:06Z

/assign @cloudbehl @afreen23

rawagner · 2019-08-29T14:01:05Z

frontend/public/components/dashboard/details-card/detail-item.tsx

hm, seems to me that the contract is a bit unclear. I would remove the value prop and just keep the children. I know it will mean that we need to update all usages, but we dont have that many yet so better do it sooner than later.

+1 removing value . Using children sounds good for future changes like this

afreen23 · 2019-08-29T18:36:52Z

frontend/packages/noobaa-storage-plugin/src/components/details-card/details-card.tsx

there is already a getMetric function in utils.ts. We must reuse that here.

Can we implement something like this:

const noobaaSystemAddress = getMetric(queryResult, 'system_address'); const noobaaSystemName = getMetric(queryResult, 'system_name'); let link = null; if (noobaaSystemAddress && noobaaSystemName) link = `${noobaaSystemAddress}/fe/systems/${noobaaSystemName}/buckets/data-buckets`;

We don't need to pass then the error prop

Ideally these lines should be wrapped in a function now!

@afreen23 will use the function for metric. However for the other link issue, I need to use the error prop because if link is undefined (which is passed through ExternalLink) then DetailItem will not figure out that the values are missing.

afreen23 · 2019-08-29T19:46:04Z

frontend/public/components/dashboard/details-card/detail-item.tsx

+1 removing value . Using children sounds good for future changes like this

lizsurette · 2019-08-29T19:57:16Z

I don't remember seeing this in the latest design:
http://openshift.github.io/openshift-origin-design/web-console/4.0-designs/storage/object-storage-dashboard/object-storage-dashboard

@shirimordechay @julienlim - Is this how you would expect a user to jump to the NooBaa console?

afreen23 · 2019-08-29T20:15:21Z

I don't remember seeing this in the latest design:
http://openshift.github.io/openshift-origin-design/web-console/4.0-designs/storage/object-storage-dashboard/object-storage-dashboard

@shirimordechay @julienlim - Is this how you would expect a user to jump to the NooBaa console?

@lizsurette
We are following invision mockups for the UI implementations and it's updated there!.

lizsurette · 2019-08-29T20:18:47Z

@lizsurette
We are following invision mockups for the UI implementations and it's updated there!.

Thanks for the updated link, @afreen23 !!

cloudbehl · 2019-08-30T05:04:09Z

/ok-to-test

cloudbehl · 2019-08-30T12:43:52Z

/approve
@rawagner have a look once again.

frontend/packages/noobaa-storage-plugin/src/components/details-card/details-card.tsx

frontend/public/components/dashboard/details-card/detail-item.tsx

rawagner · 2019-08-30T12:57:29Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

can you extract the getName(_.get(cephClusterData, 0)) so we dont have to call it twice for error and value ?
Same for other values (in other details cards too)

bipuladh · 2019-09-02T11:36:24Z

/test e2e-aws-console

bipuladh · 2019-09-02T13:44:46Z

/test e2e-aws-console

bipuladh · 2019-09-03T04:01:57Z

/test e2e-aws-console

cloudbehl · 2019-09-03T10:09:41Z

/approve
/lgtm

cloudbehl · 2019-09-03T13:41:17Z

@vojtechszocs can you review this PR.

vojtechszocs · 2019-09-03T16:28:50Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

@rawagner Please be advised, @jtomasek adds this to console-shared selectors in his PR #2558.

https://github.com/openshift/console/pull/2558/files#diff-25304ce4310324e88754e9516a46ac5c

Once #2558 is merged, we should update our existing code to use this shared selector.

This also applies to code below.

ok, I will update the existing code then

Hmm, I'm wondering if there are any duplicate code detectors for TypeScript / JavaScript, which can calculate code similarity (e.g. percentage) between files.

vojtechszocs · 2019-09-03T16:33:12Z

frontend/packages/noobaa-storage-plugin/src/components/details-card/details-card.tsx

The systemError variable below should handle such case, unless I'm missing something.

The systemLink variable could be defined as

const systemLink = !systemError ? `${systemAddress}fe/systems/${systemName}` : undefined;

to make it clear that systemLink as a whole is undefined unless both systemName and systemAddress are valid.

Anyway, I'd suggest to simplify it further, just use

const systemLink = (systemName && systemAddress) ? `${systemAddress}fe/systems/${systemName}` : undefined;

and in JSX code below

error={!systemLink}

vojtechszocs · 2019-09-03T16:42:34Z

frontend/packages/noobaa-storage-plugin/src/components/details-card/details-card.tsx

@rawagner If DetailItem has error prop set to a truthy value, does the child content (i.e. ExternalLink above) actually render?

If yes, the systemLink will be either invalid or undefined (see my suggestion above).

no, child is not rendered then. DetailItem will render unavailable string

Good, so we can simply use the error={!systemLink} as a guard for the system link 😃

vojtechszocs · 2019-09-03T16:48:57Z

frontend/public/components/dashboard/details-card/detail-item.tsx

FYI there is a React helper type to avoid repeating children in your component props:

// node_modules/@types/react/index.d.ts type PropsWithChildren<P> = P & { children?: ReactNode };

You can use it like so:

type DetailItemProps = React.PropsWithChildren<{ title: string; isLoading: boolean; error: boolean; }>;

and it will add an optional children prop.

Good to know

React.FC already includes optional children prop as it uses that helper internally, so it is not necessary to deal with children prop at all (except for the cases when we want to explicitly define children prop as specific type or make it required)

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L495

jtomasek · 2019-09-04T06:30:25Z

frontend/public/components/dashboard/details-card/detail-item.tsx

As mentioned here #2520 (comment), React.PropsWithChildren is not needed as it is already included in React.FC

frontend/public/components/dashboard/details-card/detail-item.tsx

Ran lint fix Fixing conflicts

christianvogt · 2019-09-04T20:22:41Z

/lgtm

openshift-ci-robot · 2019-09-04T20:22:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, christianvogt, cloudbehl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-09-05T00:01:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-09-05T01:19:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-09-05T02:12:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-09-05T03:03:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

bipuladh · 2019-09-05T05:58:16Z

/test e2e-aws

openshift-ci-robot added component/core Related to console core functionality component/noobaa Related to noobaa-storage-plugin needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 28, 2019

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 28, 2019

openshift-ci-robot requested review from afreen23 and zherman0 August 28, 2019 11:17

afreen23 reviewed Aug 28, 2019

View reviewed changes

bipuladh commented Aug 28, 2019

View reviewed changes

bipuladh force-pushed the noobaa_ext_link branch from 7bc578b to 53371f7 Compare August 29, 2019 13:49

openshift-ci-robot assigned afreen23 and cloudbehl Aug 29, 2019

rawagner reviewed Aug 29, 2019

View reviewed changes

afreen23 reviewed Aug 29, 2019

View reviewed changes

openshift-ci-robot added component/ceph Related to ceph-storage-plugin size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 30, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 30, 2019

rawagner suggested changes Aug 30, 2019

View reviewed changes

rawagner mentioned this pull request Sep 2, 2019

[metal3] Add details card to baremetal host dashboard #2553

Merged

bipuladh force-pushed the noobaa_ext_link branch from f222591 to e7f3e1e Compare September 2, 2019 09:10

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2019

vojtechszocs suggested changes Sep 3, 2019

View reviewed changes

bipuladh force-pushed the noobaa_ext_link branch from 8d05108 to 4a126ad Compare September 4, 2019 05:55

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2019

jtomasek reviewed Sep 4, 2019

View reviewed changes

bipuladh commented Sep 4, 2019

View reviewed changes

frontend/public/components/dashboard/details-card/detail-item.tsx Outdated Show resolved Hide resolved

rawagner mentioned this pull request Sep 4, 2019

dashboard: Enhance DetailsCard for new fields #2586

Merged

1 task

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 4, 2019

bipuladh force-pushed the noobaa_ext_link branch from 6c6b89c to 9ed54e5 Compare September 4, 2019 19:59

Added a external link to noobaa details card

3a02c4b

Ran lint fix Fixing conflicts

bipuladh force-pushed the noobaa_ext_link branch from 9ed54e5 to 3a02c4b Compare September 4, 2019 20:06

openshift-ci-robot assigned christianvogt Sep 4, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2019

openshift-merge-robot merged commit beab439 into openshift:master Sep 5, 2019

mareklibra mentioned this pull request Sep 5, 2019

kubevirt: Add VM dashboard #2600

Merged

1 task

spadgett added this to the v4.2 milestone Sep 6, 2019

Added a external link to noobaa details card #2520

Added a external link to noobaa details card #2520

Uh oh!

Conversation

bipuladh commented Aug 28, 2019

Uh oh!

openshift-ci-robot commented Aug 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh commented Aug 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afreen23 Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizsurette commented Aug 29, 2019

Uh oh!

afreen23 commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizsurette commented Aug 29, 2019

Uh oh!

cloudbehl commented Aug 30, 2019

Uh oh!

cloudbehl commented Aug 30, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bipuladh commented Sep 2, 2019

Uh oh!

bipuladh commented Sep 2, 2019

Uh oh!

bipuladh commented Sep 3, 2019

Uh oh!

cloudbehl commented Sep 3, 2019

Uh oh!

cloudbehl commented Sep 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

afreen23 Aug 28, 2019 •

edited

Loading

afreen23 Aug 29, 2019 •

edited

Loading

bipuladh Aug 28, 2019 •

edited

Loading

afreen23 Aug 29, 2019 •

edited

Loading

afreen23 commented Aug 29, 2019 •

edited

Loading