-
Notifications
You must be signed in to change notification settings - Fork 29
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
Policy configuration view for application #903
Policy configuration view for application #903
Conversation
c162629
to
6dd8516
Compare
Screencast.from.2023-07-04.15-36-53.webm |
6dd8516
to
fc0d502
Compare
fc0d502
to
6fbd11b
Compare
cb97cea
to
3a35a7e
Compare
@@ -61,7 +68,7 @@ export const AppManagePoliciesModal: React.FC<AppManagePoliciesModalProps> = ({ | |||
onClose={close} | |||
> | |||
{loaded && !loadError ? ( | |||
state.modalViewContext === ModalViewContext.POLICY_LIST_VIEW && ( | |||
(state.modalViewContext === ModalViewContext.POLICY_LIST_VIEW && ( |
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.
This could be a component by itself. We could pass all the props to one component and based on on the modalViewContext it would use the right 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.
ack
[ | ||
ModalActionContext.UN_ASSIGN_POLICIES_SUCCEEDED, | ||
ModalActionContext.UN_ASSIGN_POLICIES_FAILED, | ||
ModalActionContext.UN_ASSIGN_POLICY_SUCCEEDED, |
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.
ModalActionContext.UN_ASSIGN_POLICY_SUCCEEDED, | |
ModalActionContext.UNASSIGN_POLICY_SUCCEEDED, |
nit
id="placement-control-view-dropdown" | ||
selectedKey={selected} | ||
isDisabled={isDisabledSelector} | ||
selectOptions={dropdownOptions( |
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.
dropdownOptions
are getting recreated every re-render move it out to a variable.
onChange={(value: string) => { | ||
setSelected(value); | ||
onSelect(value); | ||
}} |
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.
Move it out to a function.
setSelected(value); | ||
onSelect(value); | ||
}} | ||
className="mco-manage-policies__dropdown--width" |
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.
className="mco-manage-policies__dropdown--width" | |
className="mco-manage-policies__dropdown--wide" |
Modifier should be an adjective.
@@ -11,4 +11,7 @@ | |||
justify-content: space-between; | |||
padding: 0 var(--pf-global--spacer--md) 0 var(--pf-global--spacer--md); | |||
} | |||
&__dropdown--width { | |||
min-width: 15rem; |
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.
Can we use PF global spacers?
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.
PF global spacers has upto 4x which is 5rem
placementControlInfo: PlacementControlProps, | ||
selected: string, | ||
isDefaultSelected: boolean | ||
) => |
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 add return types.
}; | ||
|
||
export type PlacementControlProps = { | ||
[placementName in string]: { |
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.
[placementName in string]: { | |
[placementName: string]: { |
{} | ||
); | ||
|
||
const generatefooterButtons = (props: FooterProps): FooterButtonProps => ({ |
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.
const generatefooterButtons = (props: FooterProps): FooterButtonProps => ({ | |
const generatefooterButtons = ({t, onBack}): FooterButtonProps => ({ |
<Messages state={state} /> | ||
</ModalBody> | ||
<ModalFooter> | ||
{generatefooterButtons({ |
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.
GenerateFooterButtons should be a component. This essentially recreates your components everytime in a re-render.
935880b
to
b9c3b33
Compare
(state.modalViewContext === ModalViewContext.POLICY_LIST_VIEW && ( | ||
<PolicyListView | ||
dataPolicyInfo={applicaitonInfo?.dataPolicies} | ||
state={state.policyListView} | ||
dispatch={dispatch} | ||
setModalContext={setModalContext} | ||
setModalActionContext={setModalActionContext} | ||
setMessage={setMessage} | ||
/> | ||
)) || | ||
(state.modalViewContext === ModalViewContext.UNASSIGN_POLICY_VIEW && ( | ||
<UnAssignPolicyView | ||
state={state.unAssignPolicyView} | ||
dispatch={dispatch} | ||
setModalContext={setModalContext} | ||
setModalActionContext={setModalActionContext} | ||
setMessage={setMessage} | ||
/> | ||
)) | ||
); |
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.
nits (to save OR/AND conditions):
(state.modalViewContext === ModalViewContext.POLICY_LIST_VIEW && ( | |
<PolicyListView | |
dataPolicyInfo={applicaitonInfo?.dataPolicies} | |
state={state.policyListView} | |
dispatch={dispatch} | |
setModalContext={setModalContext} | |
setModalActionContext={setModalActionContext} | |
setMessage={setMessage} | |
/> | |
)) || | |
(state.modalViewContext === ModalViewContext.UNASSIGN_POLICY_VIEW && ( | |
<UnAssignPolicyView | |
state={state.unAssignPolicyView} | |
dispatch={dispatch} | |
setModalContext={setModalContext} | |
setModalActionContext={setModalActionContext} | |
setMessage={setMessage} | |
/> | |
)) | |
); | |
{ | |
{ | |
[ModalViewContext.POLICY_LIST_VIEW]: ( | |
<PolicyListView | |
dataPolicyInfo={applicaitonInfo?.dataPolicies} | |
state={state.policyListView} | |
dispatch={dispatch} | |
setModalContext={setModalContext} | |
setModalActionContext={setModalActionContext} | |
setMessage={setMessage} | |
/> | |
), | |
[ModalViewContext.UNASSIGN_POLICY_VIEW ]: ( | |
<UnAssignPolicyView | |
state={state.unAssignPolicyView} | |
dispatch={dispatch} | |
setModalContext={setModalContext} | |
setModalActionContext={setModalActionContext} | |
setMessage={setMessage} | |
/> | |
), | |
}[state.modalViewContext] | |
} |
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.
switch case in JSX...
isn't "View config" and "Unassign" opening exactly same UI, only diff is that in latter we have an extra button to un-assign the policy/policies ?? can't we have single UI for both, in "Unassign" page they can view details as well IMO. we might not want to change UX at this point of time, still asking in case there was any specific reason for that ?? |
@@ -0,0 +1,238 @@ | |||
import * as React from 'react'; | |||
import { pluralize } from '@odf/core/components/utils'; |
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.
mco
was only supposed to be dependant upon @odf/shared
not on @odf/core
(as per dependencies in package.json of mco)...
but anyway I can see multiple such imports in mco
from @odf/core
, so guess no point to make any change for this in the PR...
): string[] => | ||
isDefaultSelected | ||
? // Aggregate all labels | ||
Object.values(placementControlInfo)?.reduce( |
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 are overusing ?.
everywhere (in general in mco)... we can keep default value of placementControlInfo
as {}
and avoid using ?.
here...
same is true for other places as well, where it is possible to handle the cases like that...
Object.values(placementControlInfo)?.reduce( | |
Object.values(placementControlInfo).reduce( |
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.
MCO is a parser logic, The problem here is, We are not reading one exact CR type. We are grouping one CR based on another, So the entire CR can be optional here. It may come or may not, so it is difficult for us to decide on an object type as these fields are mandatory and these fields are optional. or we need to declare all types with actual CR type | {}
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 will try to re-evaluate on this PR, i will check and remove it any unnecessary options symbol used
placementControls?.reduce( | ||
(acc, drpc) => ({ | ||
...acc, | ||
[getName(drpc?.placementInfo)]: drpc, | ||
}), | ||
{} | ||
); |
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.
placementControls?.reduce( | |
(acc, drpc) => ({ | |
...acc, | |
[getName(drpc?.placementInfo)]: drpc, | |
}), | |
{} | |
); | |
placementControls?.reduce( | |
(acc, drpc) => ({ | |
...acc, | |
[getName(drpc?.placementInfo)]: drpc, | |
}), | |
{} | |
) || {}; |
export const DataPolicyStatus: React.FC<DataPolicyStatusProps> = ({ | ||
isValidated, | ||
t, | ||
}) => { | ||
return ( | ||
<StatusIconAndText | ||
{...(isValidated | ||
? { title: t('Validated'), icon: <GreenCheckCircleIcon /> } | ||
: { | ||
title: t('Not Validated'), | ||
icon: <RedExclamationCircleIcon />, | ||
})} | ||
/> | ||
); | ||
}; |
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.
do we need to create a separate FC for this ?? It is just adding 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.
We are using this in more than one place, I did this to reuse the same code.
<div className="mco-manage-policies__row--padding"> | ||
<div className="mco-manage-policies__header"> | ||
<Text component="h3"> {t('Policy configuration details')} </Text> | ||
{!!Object.keys(placementControlMap)?.length && ( |
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.
{!!Object.keys(placementControlMap)?.length && ( | |
{!!Object.keys(placementControlMap).length && ( |
const dropdownOptions = React.useMemo( | ||
() => | ||
getDropdownOptions( | ||
Object.keys(placementControlMap), | ||
defaultSelectionText | ||
), | ||
[placementControlMap, defaultSelectionText] | ||
); |
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.
nits: we don;t really need to memoize it, SingleSelectDropdown
does need it's ref to be constant...
we can directly pass selectOptions={getDropdownOptions(Object.keys(placementControlMap), defaultSelectionText)}
as prop...
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.
but only a suggestion, feel free to ignore this comment if want to...
const syncScheduleFormat = { | ||
m: t('minutes'), | ||
h: t('hours'), | ||
d: t('days'), | ||
}; |
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 are using this at one more place in mco, can be moved to constants ?
<DescriptionListItem | ||
term={t('Application resources protected')} | ||
description={pluralize( | ||
Object.keys(placementControlMap)?.length, |
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.
Object.keys(placementControlMap)?.length, | |
Object.keys(placementControlMap).length, |
@@ -97,7 +98,7 @@ const generateDRPlacementControlInfo = ( | |||
metadata: drpc.metadata, | |||
drPolicyRef: drpc.spec.drPolicyRef, | |||
placementInfo: plsInfo, | |||
pvcSelector: drpc.spec?.pvcSelector, | |||
pvcSelector: arrayify(drpc?.spec?.pvcSelector?.matchLabels) || [], |
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.
do we need ?.
at every level ??
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.
no need, only for drpc is fine
@@ -44,6 +44,7 @@ export const SingleSelectDropdown: React.FC<SingleSelectDropdownProps> = ({ | |||
placeholderText={props?.placeholderText || t('Select options')} | |||
aria-labelledby={props?.id} | |||
noResultsFoundText={t('No results found')} | |||
isDisabled={props?.isDisabled} |
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.
no need...
isDisabled={props?.isDisabled} |
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 are already doing {...props}
above, so just pass isDisabled
to SingleSelectDropdown
and it will pass it down to Select
.
packages/shared/labels/labels.tsx
Outdated
labels, | ||
collapsedText, | ||
expandedText, | ||
numLabels, | ||
className, |
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.
labels, | |
collapsedText, | |
expandedText, | |
numLabels, | |
className, | |
labels, | |
labelClassName, | |
...props |
packages/shared/labels/labels.tsx
Outdated
numLabels={numLabels} | ||
expandedText={expandedText} | ||
collapsedText={collapsedText} |
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.
numLabels={numLabels} | |
expandedText={expandedText} | |
collapsedText={collapsedText} | |
{...props} |
packages/shared/labels/labels.tsx
Outdated
collapsedText={collapsedText} | ||
> | ||
{labels.map((label) => ( | ||
<Label key={label} className={className} isTruncated> |
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.
<Label key={label} className={className} isTruncated> | |
<Label key={label} className={labelClassName} isTruncated> |
packages/shared/labels/labels.tsx
Outdated
/** Customizable "Show Less" text string */ | ||
collapsedText?: string; | ||
/** Customizeable template string. Use variable "${remaining}" for the overflow label count. */ | ||
expandedText?: string; | ||
/** Set number of labels to show before overflow */ | ||
numLabels?: number; |
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.
/** Customizable "Show Less" text string */ | |
collapsedText?: string; | |
/** Customizeable template string. Use variable "${remaining}" for the overflow label count. */ | |
expandedText?: string; | |
/** Set number of labels to show before overflow */ | |
numLabels?: number; |
return modalActionContext === ModalActionContext.UN_ASSIGNING_POLICY ? ( | ||
<> | ||
<Button | ||
id="modal-cancel-action" | ||
variant={ButtonVariant.secondary} | ||
onClick={onCancel} | ||
> | ||
{t('Cancel')} | ||
</Button> | ||
<Button | ||
id="modal-confirm-action" | ||
variant={ButtonVariant.primary} | ||
onClick={unAssignPolicy} | ||
isLoading={isButtonLoading} | ||
> | ||
{t('Confirm')} | ||
</Button> | ||
</> | ||
) : ( | ||
<> | ||
<Button | ||
id="modal-back-action" | ||
variant={ButtonVariant.secondary} | ||
onClick={onBack} | ||
> | ||
{t('Back')} | ||
</Button> | ||
<Button | ||
id="modal-unassign-action" | ||
variant={ButtonVariant.primary} | ||
onClick={onUnassign} | ||
> | ||
{isAllSelected ? t('Unassign for all') : t('Unassign')} | ||
</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.
nits (but feel free to ignore this comment if want to):
return modalActionContext === ModalActionContext.UN_ASSIGNING_POLICY ? ( | |
<> | |
<Button | |
id="modal-cancel-action" | |
variant={ButtonVariant.secondary} | |
onClick={onCancel} | |
> | |
{t('Cancel')} | |
</Button> | |
<Button | |
id="modal-confirm-action" | |
variant={ButtonVariant.primary} | |
onClick={unAssignPolicy} | |
isLoading={isButtonLoading} | |
> | |
{t('Confirm')} | |
</Button> | |
</> | |
) : ( | |
<> | |
<Button | |
id="modal-back-action" | |
variant={ButtonVariant.secondary} | |
onClick={onBack} | |
> | |
{t('Back')} | |
</Button> | |
<Button | |
id="modal-unassign-action" | |
variant={ButtonVariant.primary} | |
onClick={onUnassign} | |
> | |
{isAllSelected ? t('Unassign for all') : t('Unassign')} | |
</Button> | |
</> | |
); | |
}; | |
const isContextUnAssign = modalActionContext === ModalActionContext.UN_ASSIGNING_POLICY; | |
return ? ( | |
<> | |
<Button | |
id="modal-cancel-action" | |
variant={ButtonVariant.secondary} | |
onClick={isContextUnAssign ? onCancel : onBack} | |
> | |
{isContextUnAssign ? t('Cancel') : t('Back')} | |
</Button> | |
<Button | |
id="modal-confirm-action" | |
variant={ButtonVariant.primary} | |
onClick={isContextUnAssign ? unAssignPolicy: onUnassign} | |
isLoading={isButtonLoading} | |
> | |
{isContextUnAssign ? ('Confirm') : isAllSelected ? t('Unassign for all') : t('Unassign')} | |
</Button> | |
</> | |
) | |
}; |
It's a single UI only, the only difference is I change the footer using generate footer component. |
b9c3b33
to
a9e8840
Compare
packages/shared/src/utils/common.ts
Outdated
) => { | ||
const pluralized = `${count === 1 ? singular : plural}`; | ||
return includeCount ? `${count || 0} ${pluralized}` : pluralized; | ||
}; |
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 have created a new pluralize function in shared and referring from here,
later we remove the old pluralize function reference from all places.
a9e8840
to
010d51c
Compare
010d51c
to
919c1a1
Compare
const SCHEDULE_FORMAT = { | ||
[SyncScheduleFormat.minutes]: TIME_UNITS.Minutes, | ||
[SyncScheduleFormat.hours]: TIME_UNITS.Hours, | ||
[SyncScheduleFormat.days]: TIME_UNITS.Days, | ||
[SyncScheduleFormat.m]: TIME_UNITS.Minutes, | ||
[SyncScheduleFormat.h]: TIME_UNITS.Hours, | ||
[SyncScheduleFormat.d]: TIME_UNITS.Days, | ||
}; |
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.
this doesn't need to be part of the react component.
}; | ||
|
||
const [isOpen, setIsOpen] = React.useState(false); | ||
const [selectedFormat, setSelectedFormat] = React.useState( | ||
SyncScheduleFormat.minutes | ||
SyncScheduleFormat.m |
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.
Can we keep it as original to minutes, hours and days? m
could be month or minute.
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 a unit from DRPolicy CR, DRPolicy is using d, m, h to represent sync interval, in UI we are mapping
m to minutes
h to hours
d to days
|
||
export const AppManagePoliciesModal: React.FC<AppManagePoliciesModalProps> = ({ | ||
export const ModalBody: React.FC<ModalBodyProps> = ({ |
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.
ModalBody
is too generic name. can we make it application specific.
modalActionViewContext: ModalActionContext, | ||
modalViewContext?: ModalViewContext |
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.
modalActionViewContext: ModalActionContext, | |
modalViewContext?: ModalViewContext | |
modalActionContext: ModalActionContext, | |
modalViewContext?: ModalViewContext |
[ModalViewContext.POLICY_LIST_VIEW]: ( | ||
<PolicyListView | ||
dataPolicyInfo={applicaitonInfo?.dataPolicies} | ||
state={state.policyListView} | ||
dispatch={dispatch} | ||
setModalContext={setModalContext} | ||
setModalActionContext={setModalActionContext} | ||
setMessage={setMessage} | ||
/> | ||
), | ||
[ModalViewContext.POLICY_CONFIGURATON_VIEW]: ( | ||
<PolicyConfigView | ||
state={state.policyConfigurationView} | ||
setModalContext={setModalContext} | ||
/> | ||
), | ||
}[state.modalViewContext]; |
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.
You are creating an object and then passing a key to the object. This will not be stable. You could select a component first based on the value of modalViewContext
and then render it.
const dropdownOptions = getDropdownOptions( | ||
Object.keys(placementControlMap), | ||
defaultSelectionText | ||
); |
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.
Do you want to memoize it?
919c1a1
to
9833a77
Compare
const setModalContext = (modalViewContext: ModalViewContext) => { | ||
dispatch({ | ||
type: ManagePolicyStateType.SET_MODAL_VIEW_CONTEXT, | ||
payload: modalViewContext, | ||
}); | ||
}; | ||
|
||
const setModalActionContext = (modalViewContext: ModalActionContext) => | ||
const setModalActionContext = ( |
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.
Wrap it in useCallback
}); | ||
|
||
const setMessage = (message: MessageType) => { | ||
const setMessage = ( |
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.
wrap it in useCallback
<ModalBody> | ||
<PolicyConfigViewer policy={state.policy} /> | ||
</ModalBody> | ||
<ModalFooter> |
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 only have one 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.
9833a77
to
55e9c22
Compare
export type LabelsProps = LableGroup & { | ||
labels: string[]; | ||
labelClassName?: string; | ||
}; |
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.
each directory under src
will have an index.ts
file which will export the utility, here all export from labels.tsx
file...
then there is an index.ts
file directly under src
need to export from there as well...
55e9c22
to
ca39b18
Compare
packages/shared/src/index.ts
Outdated
@@ -13,6 +13,7 @@ export * from './heading'; | |||
export * from './hooks'; | |||
export * from './input-with-requirements'; | |||
export * from './kebab'; | |||
export * from './labels/labels'; |
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.
export * from './labels/labels'; | |
export * from './labels'; |
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.
label directory will also have an index file...
ca39b18
to
797d520
Compare
@@ -0,0 +1,34 @@ | |||
import * as React from 'react'; | |||
import { Label, LabelGroup } from '@patternfly/react-core'; |
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.
import { Label, LabelGroup } from '@patternfly/react-core'; | |
import { Label, LabelGroup, LabelGroupProps } from '@patternfly/react-core'; |
export type LableGroup = { | ||
/** Customizable "Show Less" text string */ | ||
collapsedText?: string; | ||
/** Customizeable template string. Use variable "${remaining}" for the overflow label count. */ | ||
expandedText?: string; | ||
/** Set number of labels to show before overflow */ | ||
numLabels?: number; | ||
}; | ||
|
||
export type LabelsProps = LableGroup & { | ||
labels: string[]; | ||
labelClassName?: string; | ||
}; |
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.
export type LableGroup = { | |
/** Customizable "Show Less" text string */ | |
collapsedText?: string; | |
/** Customizeable template string. Use variable "${remaining}" for the overflow label count. */ | |
expandedText?: string; | |
/** Set number of labels to show before overflow */ | |
numLabels?: number; | |
}; | |
export type LabelsProps = LableGroup & { | |
labels: string[]; | |
labelClassName?: string; | |
}; | |
export type LabelsProps = LabelGroupProps & { | |
labels: string[]; | |
labelClassName?: string; | |
}; |
Signed-off-by: Gowtham Shanmugasundaram <[email protected]>
797d520
to
b345d5f
Compare
/lgtm |
cc @bipuladh for approval... |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, GowthamShanmugam 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 |
On top of #894