Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Update the open dialog API with more options.#14

Merged
richvdh merged 5 commits intomatrix-org:mainfrom
nordeck:module_dialog_update
Aug 16, 2023
Merged

Update the open dialog API with more options.#14
richvdh merged 5 commits intomatrix-org:mainfrom
nordeck:module_dialog_update

Conversation

@maheichyk
Copy link
Contributor

This PR suggest to update module API with more options to control rendering of the dialog shown:

  1. Enable/disable dialog submit button.
  2. Custom labels for cancel and submit (action) buttons.

Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. I have a few requests.

@dhenneke
Copy link
Contributor

@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.

@richvdh
Copy link
Member

richvdh commented Aug 10, 2023

Context on this PR: it seems to be documenting the changes made to openDialog in matrix-org/matrix-react-sdk#10536 matrix-org/matrix-react-sdk#11395

@richvdh richvdh self-requested a review August 10, 2023 17:03
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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,
Copy link
Contributor

@dhenneke dhenneke Aug 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, no idea :(

Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@dhenneke
Copy link
Contributor

@richvdh Thanks for the feedback. I applied your suggestions and slightly tweaked them.

@richvdh richvdh self-requested a review August 14, 2023 13:01
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work on this; I think it's ready to merge!

@richvdh richvdh merged commit 2e091fa into matrix-org:main Aug 16, 2023
@dhenneke dhenneke deleted the module_dialog_update branch August 16, 2023 12:41
@dhenneke
Copy link
Contributor

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.

@richvdh
Copy link
Member

richvdh commented Aug 16, 2023

Done: https://github.com/matrix-org/matrix-react-sdk-module-api/releases/tag/v2.0.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants