feat(WarningModal): add text confirmation option by InsaneZein · Pull Request #540 · patternfly/react-component-groups

InsaneZein · 2025-01-07T20:10:17Z

closes #411

for RHCLOUD-36638: adds a text confirmation option to the Warning Modal

patternfly-build · 2025-01-07T20:12:03Z

Preview: https://react-component-groups-pr-component-groups-540.surge.sh

A11y report: https://react-component-groups-pr-component-groups-540-a11y.surge.sh

fhlavac · 2025-01-08T17:45:57Z

packages/module/src/WarningModal/WarningModal.tsx

+  /** Whether modal requires a text confirmation */
+  textConfirmation?: TextInputProps;
+  /** Label for the text confirmation */
+  textConfirmationLabel?: (deleteName?: string) => ReactNode;
+  /** Text the user should type to confirm selection when using textConfirmation */
+  deleteName?: string;


Suggested change

/** Whether modal requires a text confirmation */

textConfirmation?: TextInputProps;

/** Label for the text confirmation */

textConfirmationLabel?: (deleteName?: string) => ReactNode;

/** Text the user should type to confirm selection when using textConfirmation */

deleteName?: string;

/** Confirmation text input props */

confirmationInputProps?: TextInputProps;

/** Label for the text confirmation input */

confirmationInputLabel?: (deleteName?: string) => ReactNode;

/** Text the user should type to confirm selection when using confirmation input */

confirmationText?: string;

how about renaming the props like so? Mabe it's a little bit more descriptive

fhlavac · 2025-01-08T17:48:41Z

packages/module/src/WarningModal/WarningModal.tsx

+  const isConfirmButtonDisabled = () => {
+    if(withCheckbox && textConfirmation) {
+      return !checked || !textConfirmed;
+    }
+    if(withCheckbox && !textConfirmation) {
+      return !checked;
+    }
+    if(!withCheckbox && textConfirmation) {
+      return !textConfirmed;
+    }
+    else {return false;}
+  }


Suggested change

const isConfirmButtonDisabled = () => {

if(withCheckbox && textConfirmation) {

return !checked || !textConfirmed;

}

if(withCheckbox && !textConfirmation) {

return !checked;

}

if(!withCheckbox && textConfirmation) {

return !textConfirmed;

}

else {return false;}

}

const isConfirmButtonDisabled = React.useMemo(() => {

if (withCheckbox) {

return !checked || (textConfirmation && !textConfirmed);

}

return textConfirmation ? !textConfirmed : false;

}, [checked, textConfirmed, withCheckbox, textConfirmation]);

fhlavac · 2025-01-08T17:52:26Z

packages/module/src/WarningModal/WarningModal.tsx

            setChecked(false);
+            setInputValue('');


is this cleanup needed? it causes unnecessary re-renders.

fhlavac · 2025-01-08T17:56:27Z

...le/patternfly-docs/content/extensions/component-groups/examples/WarningModal/WarningModal.md

+
+### Warning modal with a text confirmation
+
+To confirm that a user wishes to initiate a selected action, you can add a text confirmation.


Suggested change

To confirm that a user wishes to initiate a selected action, you can add a text confirmation.

To ensure the user intentionally initiates a selected action, you can add a confirmation text input.

fhlavac · 2025-01-08T17:59:42Z

cypress/component/WarningModal.cy.tsx

+      cancelButtonLabel='No'
+      onClose={() => setIsOpen(false)}
+      onConfirm={() => setIsOpen(false)}
+      textConfirmation={{ type: 'text', isRequired: true }}


maybe these two props could be set as default and made overwritable using custom input props

If we wanted to do this, I think we'd have to add something like a withTextConfirmation boolean prop for this to work.

fhlavac · 2025-01-08T18:00:33Z

@InsaneZein just some little suggestions. Otherwise, it looks great!

fhlavac · 2025-01-10T09:06:56Z

packages/module/src/WarningModal/WarningModal.tsx

+                ouiaId={`${ouiaId}-confirmation-text-input`}
+                value={inputValue}
+                onChange={(_e, value) => setInputValue(value)}
+                {...confirmationInputProps}


@InsaneZein for the input default props mentioned above, we could do this.
What do you think?

Suggested change

{...confirmationInputProps}

{{ type: 'text', isRequired: true, ...confirmationInputProps}}

fhlavac · 2025-01-10T09:09:14Z

packages/module/src/WarningModal/WarningModal.tsx

+          {confirmationInputProps ? (
+            <Flex direction={{ default: 'column' }} spaceItems={{ default: 'spaceItemsSm' }}>
+              <FlexItem>
+                {confirmationInputProps && confirmationInputLabel(deleteNameSanitized)}


Suggested change

{confirmationInputProps && confirmationInputLabel(deleteNameSanitized)}

{confirmationInputLabel(deleteNameSanitized)}

the check is probably not needed, since you have it above

fhlavac · 2025-01-10T09:11:28Z

packages/module/src/WarningModal/WarningModal.tsx

+  /** Label for the text confirmation input */
+  confirmationInputLabel?: (deleteName?: string) => ReactNode;


not sure if this one has to be a function - if the user of our component uses this prop, he can pass the confirmation text directly, not using another prop. I would just make it a ReactNode

fhlavac

Two final comments and we are golden! 🎉

fhlavac · 2025-01-13T09:29:52Z

packages/module/src/WarningModal/WarningModal.tsx

+      <Stack hasGutter>
+        <StackItem>{children}</StackItem>
+        <StackItem>
+          {confirmationInputProps ? (


Suggested change

{confirmationInputProps ? (

{confirmationText ? (

we probably want to show the confirmation input only if the text is passed, not necessarily the input props

fhlavac · 2025-01-13T09:34:54Z

packages/module/src/WarningModal/WarningModal.tsx

+  confirmationInputLabel = 'Type to confirm',
+  confirmationText,


Suggested change

confirmationInputLabel = 'Type to confirm',

confirmationText,

confirmationText,

confirmationInputLabel = <>Type <strong>{confirmationText} </strong> to confirm the action:</>,

I would keep your previous default there - it makes sense to have the confirmation text present

fhlavac

Looks great!

github-actions · 2025-01-14T14:01:42Z

🎉 This PR is included in version 6.2.0-prerelease.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(WarningModal): add text confirmation option

a29469c

InsaneZein requested a review from fhlavac January 7, 2025 20:12

fhlavac reviewed Jan 8, 2025

View reviewed changes

InsaneZein added 3 commits January 9, 2025 11:05

chore: clarify wording and clean up code

786aa67

chore: merge master

50cdc8c

test: update WarningModal snapshot

5b964f5

dlabaj approved these changes Jan 10, 2025

View reviewed changes

fhlavac approved these changes Jan 10, 2025

View reviewed changes

fhlavac reviewed Jan 10, 2025

View reviewed changes

fhlavac suggested changes Jan 10, 2025

View reviewed changes

InsaneZein added 2 commits January 10, 2025 08:29

fix(WarningModal): add default props to TextInput

ea656c3

change confirmInputLabel to ReactNode

6f5df57

fhlavac reviewed Jan 13, 2025

View reviewed changes

fix(WarningModal): reword default confirmationInputLabel

ed1afe1

fhlavac approved these changes Jan 14, 2025

View reviewed changes

chore: merge master

b62f2b2

InsaneZein merged commit c758701 into main Jan 14, 2025
6 checks passed

github-actions bot added the released on @prerelease label Jan 14, 2025

github-actions bot mentioned this pull request Apr 21, 2025

The automated release is failing 🚨 #692

Closed


		### Warning modal with a text confirmation

		To confirm that a user wishes to initiate a selected action, you can add a text confirmation.

	To confirm that a user wishes to initiate a selected action, you can add a text confirmation.
	To ensure the user intentionally initiates a selected action, you can add a confirmation text input.

	{...confirmationInputProps}
	{{ type: 'text', isRequired: true, ...confirmationInputProps}}

	{confirmationInputProps && confirmationInputLabel(deleteNameSanitized)}
	{confirmationInputLabel(deleteNameSanitized)}

		/** Label for the text confirmation input */
		confirmationInputLabel?: (deleteName?: string) => ReactNode;

		confirmationInputLabel = 'Type to confirm',
		confirmationText,

Comments

Conversation

InsaneZein commented Jan 7, 2025 • edited by fhlavac Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac commented Jan 8, 2025

Uh oh!

fhlavac Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhlavac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

InsaneZein commented Jan 7, 2025 •

edited by fhlavac

Loading

patternfly-build commented Jan 7, 2025 •

edited

Loading

fhlavac Jan 10, 2025 •

edited

Loading