Skip to content
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

Add acknowledge checkbox option #94

Conversation

MuscularSloth
Copy link
Contributor

According #92

Add an acknowledge checkbox option.
Until this checkbox is unchecked the confirm button will be disabled. Additionally you can set the custom label, custom form control props and custom checkbox props.

README.md Outdated
Comment on lines 100 to 101
| **`isAcknowledgeCheckbox`** | `boolean` | `false` | If enabled, it shows the acknowledge checkbox and disables the confirm button while the checkbox is unchecked.|
| **`acknowledgeCheckboxLabel`** | `string` | `Please confirm` | Acknowledge checkbox label |
Copy link
Owner

Choose a reason for hiding this comment

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

I would merge these two options. Let's make it acknowledgement string that is the actual label and enables the feature (or perhaps it should be arbitrary node, since Material UI allows that).

Then we have acknowledgementFormControlLabelProps and acknowledgementCheckboxProps.

/>,
);

for (let i = 0; i <= 5; i++) {
Copy link
Owner

Choose a reason for hiding this comment

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

We only need to do this twice.

Comment on lines 104 to 136
confirmationKeyword && (
<DialogContent {...contentProps}>{confirmationContent}</DialogContent>
acknowledgeCheckbox && (
<DialogContent {...contentProps}>{acknowledgeCheckbox}</DialogContent>
Copy link
Owner

Choose a reason for hiding this comment

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

We should have both in the alternative path, something like this:

(confirmationKeyword || acknowledgeCheckbox) && (
  <DialogContent {...contentProps}>
    {confirmationContent}
    {acknowledgeCheckbox}
  </DialogContent>
)

@jonatanklosko
Copy link
Owner

Looks good, just a couple comments :)

@MuscularSloth
Copy link
Contributor Author

Looks good, just a couple comments :)

@jonatanklosko Thank you for code review and suggestions! I believe all comments resolved =)

README.md Outdated
@@ -97,6 +97,9 @@ representing the user choice (resolved on confirmation and rejected on cancellat
| **`allowClose`** | `boolean` | `true` | Whether natural close (escape or backdrop click) should close the dialog. When set to `false` force the user to either cancel or confirm explicitly. |
| **`confirmationKeyword`** | `string` | `undefined` | If provided the confirmation button will be disabled by default and an additional textfield will be rendered. The confirmation button will only be enabled when the contents of the textfield match the value of `confirmationKeyword` |
| **`confirmationKeywordTextFieldProps`** | `object` | `{}` | Material-UI [TextField](https://mui.com/material-ui/api/text-field/) props for the confirmation keyword textfield. |
| **`acknowledgement`** | `boolean &#124; string` | `false` | If `true`, it shows the acknowledge checkbox with default label `Please confirm` and disables the confirm button while the checkbox is unchecked. If `string` it will show this string as checkbox label. |
Copy link
Owner

Choose a reason for hiding this comment

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

Let's make it always a string (default to undefined), it's a rather specific use case, so I think usually it's going to be a specific text as well :)

@jonatanklosko
Copy link
Owner

Great, one last comment and we can ship!

@MuscularSloth
Copy link
Contributor Author

Great, one last comment and we can ship!

Thanks for the comment!
Made these changes and removed additional value checks, tests, and storybook accordingly =)

src/ConfirmationDialog.js Outdated Show resolved Hide resolved
src/ConfirmationDialog.js Outdated Show resolved Hide resolved
src/ConfirmationDialog.js Outdated Show resolved Hide resolved
Copy link
Owner

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

Thanks!

@jonatanklosko jonatanklosko merged commit ff73669 into jonatanklosko:master Apr 4, 2024
1 check passed
@jonatanklosko
Copy link
Owner

Released in v3.0.12 :)

@MuscularSloth MuscularSloth deleted the feature/#92/add-acknowledge-checkbox branch April 9, 2024 07:58
@d12
Copy link

d12 commented Apr 16, 2024

Super sick, I was looking for exactly this and was pleasantly surprised to see it shipped just two weeks ago :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants