Update the open dialog API with more options. #14
Conversation
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
|
@richvdh Thanks for your comments and sorry for not coming back to them earlier. I applied the requested changes and changed the interface slightly, so that we can also change the labels and the title from the component if necessary. |
|
Context on this PR: it seems to be documenting the changes made to |
There was a problem hiding this comment.
I think I'm beginning to get my head round this. I think what would really help is clearer documentation about how the various classes and functions interact.
I've made some suggestions, but please check if my suggestions are accurate and make sense to you -- and feel free to improve on my suggestions too!
| * @param body The function which creates a body component for the dialog. | ||
| * @param props Optional props to provide to the dialog. | ||
| * @returns Whether the user submitted the dialog or closed it, and the model returned by the | ||
| * dialog component if submitted. | ||
| */ | ||
| openDialog<M extends object, P extends DialogProps = DialogProps, C extends DialogContent<P> = DialogContent<P>>( | ||
| title: string, | ||
| titleOrOptions: string | ModuleUiDialogOptions, | ||
| body: (props: P, ref: React.RefObject<C>) => React.ReactNode, |
There was a problem hiding this comment.
Just a comment: thb, I'm unsure why this was not defined as BodyComponent: React.Component<P>. Then the users wouldn't need to work with the ref and also the props in here would be more clear.
// could do this:
openDialog("title", DialogBody, { greeting: 'Hi!' });
// instead of this:
openDialog(
"title",
(props, ref) => (
<DialogBody
ref={ref}
{...props}
// why not add greeting="Hi!" here instead of passing it in the next line?
/>
),
{ greeting: 'Hi!' },
);But nothing I want to change (or even do without breaking compatibility).
Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
|
@richvdh Thanks for the feedback. I applied your suggestions and slightly tweaked them. |
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
|
Thanks for the review! If you could create a release of the module api, I can finish matrix-org/matrix-react-sdk#11395 as the next step. |
This PR suggest to update module API with more options to control rendering of the dialog shown: