-
Notifications
You must be signed in to change notification settings - Fork 24
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
Expose DataSource List and Details Page #681
Conversation
778a2ec
to
3fa6de7
Compare
992c717
to
9e7e43f
Compare
@@ -0,0 +1,34 @@ | |||
import * as React from 'react'; | |||
import { K8sResourceCondition } from 'src/views/clusteroverview/overview/components/details-card/utils/types'; |
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.
please fix import
@@ -0,0 +1,30 @@ | |||
import * as React from 'react'; | |||
import { K8sResourceCondition } from 'src/views/clusteroverview/overview/components/details-card/utils/types'; |
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.
please fix import
@@ -0,0 +1,44 @@ | |||
import { K8sResourceCondition } from 'src/views/clusteroverview/overview/components/details-card/utils/types'; |
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.
those auto imports... :)
'data-test-id': testId, | ||
}) => { | ||
const { t } = useKubevirtTranslation(); | ||
const NotAvailable = <MutedTextSpan text={t('Not available')} />; |
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.
i think we already have this kind of generic component... please check. @avivtur
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.
i copied @avivtur 's component and made it generic. we should use this one instead of his
const getItemHeader = () => { | ||
if (isPopover && bodyContent) { | ||
return ( | ||
<Popover | ||
headerContent={descriptionHeader} | ||
bodyContent={ | ||
<> | ||
{bodyContent} | ||
{moreInfoURL && ( | ||
<> | ||
{t('More info: ')} | ||
<Link to={moreInfoURL}>{moreInfoURL}</Link> | ||
</> | ||
)} | ||
{breadcrumb && ( | ||
<Breadcrumb> | ||
{breadcrumb.split('.').map((item) => ( | ||
<BreadcrumbItem key={item}>{item}</BreadcrumbItem> | ||
))} | ||
</Breadcrumb> | ||
)} | ||
</> | ||
} | ||
> | ||
<DescriptionListTermHelpTextButton> | ||
{' '} | ||
{descriptionHeader}{' '} | ||
</DescriptionListTermHelpTextButton> | ||
</Popover> | ||
); | ||
} | ||
|
||
return <DescriptionListTerm>{descriptionHeader}</DescriptionListTerm>; | ||
}; | ||
|
||
const description = ( | ||
<Button | ||
type="button" | ||
isInline | ||
isDisabled={isDisabled} | ||
onClick={onEditClick} | ||
variant="link" | ||
data-test-id={testId} | ||
> | ||
<Flex spaceItems={{ default: 'spaceItemsNone' }}> | ||
<FlexItem>{descriptionData ?? NotAvailable}</FlexItem> | ||
<FlexItem> | ||
<PencilAltIcon className="co-icon-space-l pf-c-button-icon--plain" /> | ||
</FlexItem> | ||
</Flex> | ||
</Button> | ||
); |
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.
consider exporting to new components, in same folder or nested folder as Title..
<div className="pf-c-page__main-breadcrumb"> | ||
<Breadcrumb className="pf-c-breadcrumb co-breadcrumb"> | ||
<BreadcrumbItem> | ||
<Button |
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.
maybe we can just use Link here?
const loading = ( | ||
<Bullseye> | ||
<Loading /> | ||
</Bullseye> | ||
); | ||
return !dataSource ? ( | ||
loading | ||
) : ( |
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.
maybe const loading is redundent? simply put content ?
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.
we should avoid popcorn loading, which is the current behavior in the vm details page, where data pops up everywhere in random timing.
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.
Wont we get the same? with or without const? i don't see a logic difference..
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.
oops i thought u were talking about another file :)
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.
I think we had some crashes so this fixed it, right @avivtur ?
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.
its was about React.suspense , not related to const loading :)
<DescriptionItem | ||
descriptionHeader={t('Managed by')} | ||
descriptionData={ | ||
dataImportCron && ( |
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.
what happen if no dataImportCron? we show empty?
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.
yes Not available
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.
awesome work! left some minor comments
9e7e43f
to
23556ab
Compare
|
23556ab
to
22d86c7
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: glekner, metalice The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 Description
Expose DataSource List and Details Page
🎥 Demo