Refactors Modal usage #971
Conversation
bipuladh
requested review from
SanjalKatiyar,
GowthamShanmugam and
vbnrh
and removed request for
GowthamShanmugam
| return !loaded ? ( | ||
| <LoadingBox /> | ||
| ) : ( |
There was a problem hiding this comment.
DetailsPage takes loaded and loadError props to internally display the LoadingBox...
| return !loaded ? ( | |
| <LoadingBox /> | |
| ) : ( | |
| return |
There was a problem hiding this comment.
This code is from before. Let's refactor this in a different PR.
| const launchModal = React.useCallback( | ||
| () => launcher(STATUS_MODAL, dataPoliciesStatus), | ||
| () => launcher(DataPoliciesStatusModal, dataPoliciesStatus), |
There was a problem hiding this comment.
don't we need to pass isOpen = true here as well ?? I don't think dataPoliciesStatus has that key...
|
|
||
| const history = useHistory(); | ||
|
|
||
| const launchModal = React.useCallback( | ||
| () => launcher(NS_STORE_MODAL_KEY, null), | ||
| () => launcher(NamespaceStoreCreateModal, null), |
There was a problem hiding this comment.
same doubt here... NamespaceStoreCreateModal is expecting isOpen prop, don't we need to pass it ?
There was a problem hiding this comment.
earlier useModalLauncher was passing it... I think now we might have to do it explicitly...
| }, | ||
| ]} | ||
| /> | ||
| ); | ||
| }, [launchModal, resource, namespace]); | ||
| }, [resource, namespace]); |
There was a problem hiding this comment.
| }, [resource, namespace]); | |
| }, [t, resource, namespace]); |
| onEdit={() => | ||
| launchModal(ModalKeys.EDIT_LABELS, { resource, resourceModel }) | ||
| } | ||
| onEdit={() => launchModal(EditLabelModal, { resource, resourceModel })} |
| @@ -358,7 +358,7 @@ export const ResourceSummary: React.FC<ResourceSummaryProps> = ({ | |||
| type="button" | |||
| isInline | |||
| onClick={() => | |||
| launchModal(ModalKeys.EDIT_ANN, { | |||
| launchModal(AnnotationsModal, { | |||
There was a problem hiding this comment.
isOpen = true ??
Ideally Modal itself should use isOpen as true by default, but okay if we don't want to make changes to modal in this PR... but we might need to pass the prop explicitly if that's the case ??
bipuladh
force-pushed
the
refactor-kebab
branch
from
5464e34
to
dea2642
Compare
|
/lgtm |
|
/retest |
bipuladh
force-pushed
the
refactor-kebab
branch
from
dea2642
to
268fb48
Compare
|
/lgtm |
|
/test odf-console-e2e-aws |
Uses useModal hook exposed as part of SDK in favour of temporary workaround Signed-off-by: Bipul Adhikari <[email protected]>
Signed-off-by: Bipul Adhikari <[email protected]>
bipuladh
force-pushed
the
refactor-kebab
branch
from
268fb48
to
9af6495
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alfonsomthd, bipuladh, SanjalKatiyar 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 |
openshift-merge-robot
merged commit 979dcae
into
red-hat-storage:master
Notable changes:
Uses useModal hook exposed as part of SDK in favour of temporary workaround
Kebab Menu internally handles launching modal and figuring out the correct modal.
Components will handle the translation of kebab menu items externally instead of passing a function that takes
tas an argument.