Repository navigation
change: [M3-7813] - Allow the disabling of the TypeToConfirm input #10251
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
Changes from all commits
c8a7399
492fab4
9de33b8
53ffd9f
d02007f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,22 +32,53 @@ interface EntityInfo { | |
| } | ||
|
|
||
| interface TypeToConfirmDialogProps { | ||
| /** | ||
| * Chidlren are rendered above the TypeToConfirm input | ||
| */ | ||
| children?: React.ReactNode; | ||
| /** | ||
| * Props to be allow disabling the input | ||
| */ | ||
| disableTypeToConfirmInput?: boolean; | ||
| /** | ||
| * Props to be allow disabling the submit button | ||
| */ | ||
| disableTypeToConfirmSubmit?: boolean; | ||
| /** | ||
| * The entity being confirmed | ||
| */ | ||
| entity: EntityInfo; | ||
| /** | ||
| * Error to be displayed in the dialog | ||
| */ | ||
| errors?: APIError[] | null | undefined; | ||
| /* | ||
| * The label for the dialog | ||
| */ | ||
| label: string; | ||
| /** | ||
| * The loading state of dialog | ||
| */ | ||
| loading: boolean; | ||
| /** | ||
| * The click handler for the primary button | ||
| */ | ||
| onClick: () => void; | ||
| /** | ||
| * The open/closed state of the dialog | ||
| */ | ||
| open: boolean; | ||
| } | ||
|
|
||
| type CombinedProps = TypeToConfirmDialogProps & | ||
| ConfirmationDialogProps & | ||
| Partial<TypeToConfirmProps>; | ||
| Partial<Omit<TypeToConfirmProps, 'disabled'>>; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This leads to confusion since we may want both the input & and the button to be disabled for different reasons |
||
|
|
||
| export const TypeToConfirmDialog = (props: CombinedProps) => { | ||
| const { | ||
| children, | ||
| disableTypeToConfirmInput, | ||
| disableTypeToConfirmSubmit, | ||
| entity, | ||
| errors, | ||
| inputProps, | ||
|
|
@@ -64,8 +95,10 @@ export const TypeToConfirmDialog = (props: CombinedProps) => { | |
| const [confirmText, setConfirmText] = React.useState(''); | ||
|
|
||
| const { data: preferences } = usePreferences(); | ||
| const disabled = | ||
| preferences?.type_to_confirm !== false && confirmText !== entity.name; | ||
| const isPrimaryButtonDisabled = | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just changing the const name to make things clearer what element is meant to be disabled by those conditions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there ever a case where we would want Iβm imagining a scenario where a user who has disabled type-to-confirm in their preferences is able to proceed with some action while a user who has type-to-confirm enabled gets blocked by the disabled input field, but I might not be understanding the logic fully
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm - trying to wrap my head around this - I don't think so? It feels to me like if the user has type-to-confirm disabled, we'd just need to rely on API errors as it is currently? Hope that answers this concern - what's implemented here really merely a visual helper so I am trying to think of it in the simplest way possible. Do you have any other suggestion as to how handling it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're changes look good. On a side note (for another ticket), I find it odd that I can't manually disable the "Delete" button in the dialog. I would imagine that if there's still Linodes assigned to the PG and type-to-confirm is disabled, the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, i hear that. This component was breaking a few things so I fixed things in 53ffd9f Mostly, the issue is that the |
||
| (preferences?.type_to_confirm !== false && confirmText !== entity.name) || | ||
| disableTypeToConfirmSubmit; | ||
| const isTypeToConfirmInputDisabled = disableTypeToConfirmInput; | ||
|
|
||
| React.useEffect(() => { | ||
| if (open) { | ||
|
|
@@ -82,7 +115,7 @@ export const TypeToConfirmDialog = (props: CombinedProps) => { | |
| <ActionsPanel | ||
| primaryButtonProps={{ | ||
| 'data-testid': 'confirm', | ||
| disabled, | ||
| disabled: isPrimaryButtonDisabled, | ||
| label: entity.primaryBtnText, | ||
| loading, | ||
| onClick, | ||
|
|
@@ -120,6 +153,7 @@ export const TypeToConfirmDialog = (props: CombinedProps) => { | |
| setConfirmText(input); | ||
| }} | ||
| data-testid={'dialog-confirm-text-input'} | ||
| disabled={isTypeToConfirmInputDisabled} | ||
| expand | ||
| hideInstructions={entity.subType === 'CloseAccount'} | ||
| inputProps={inputProps} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,6 @@ const CloseAccountDialog = ({ closeDialog, open }: Props) => { | |
| ); | ||
| const [errors, setErrors] = React.useState<APIError[] | undefined>(undefined); | ||
| const [comments, setComments] = React.useState<string>(''); | ||
| const [inputtedUsername, setUsername] = React.useState<string>(''); | ||
| const [canSubmit, setCanSubmit] = React.useState<boolean>(false); | ||
| const { classes } = useStyles(); | ||
| const history = useHistory(); | ||
| const { data: profile } = useProfile(); | ||
|
|
@@ -42,23 +40,9 @@ const CloseAccountDialog = ({ closeDialog, open }: Props) => { | |
| * intentionally not resetting comments | ||
| */ | ||
| setErrors(undefined); | ||
| setUsername(''); | ||
| setCanSubmit(false); | ||
| } | ||
| }, [open]); | ||
|
|
||
| /** | ||
| * enable the submit button if the user entered their | ||
| * username correctly | ||
| */ | ||
| React.useEffect(() => { | ||
| if (inputtedUsername === profile?.username) { | ||
| setCanSubmit(true); | ||
| } else { | ||
| setCanSubmit(false); | ||
| } | ||
| }, [inputtedUsername, profile]); | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dunno why this code was here, but it was trying to do the same thing the TypeToConfirmDialog does by default (I fixed it cause the e2e was failing) |
||
| const inputRef = React.useCallback( | ||
| (node: any) => { | ||
| /** | ||
|
|
@@ -103,7 +87,6 @@ const CloseAccountDialog = ({ closeDialog, open }: Props) => { | |
| subType: 'CloseAccount', | ||
| type: 'AccountSetting', | ||
| }} | ||
| disabled={!canSubmit} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this has 0 effect |
||
| inputRef={inputRef} | ||
| label={`Please enter your Username (${profile.username}) to confirm.`} | ||
| loading={isClosingAccount} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,13 +42,13 @@ describe('Kubernetes deletion dialog', () => { | |
| ); | ||
| const button = getByTestId('confirm'); | ||
|
|
||
| expect(button).toHaveAttribute('aria-disabled', 'true'); | ||
| expect(button).toBeDisabled; | ||
|
|
||
| await findByTestId('textfield-input'); | ||
|
|
||
| const input = getByTestId('textfield-input'); | ||
| fireEvent.change(input, { target: { value: 'this-cluster' } }); | ||
|
|
||
| expect(button).toHaveAttribute('aria-disabled', 'false'); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was a false positive! |
||
| expect(button).toBeEnabled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,10 +93,8 @@ export const PlacementGroupsDeleteModal = (props: Props) => { | |
| ? [{ reason: 'Placement Group not found.' }] | ||
| : undefined | ||
| } | ||
| inputProps={{ | ||
| disabled: isDisabled, | ||
| }} | ||
| disabled={isDisabled} | ||
| disableTypeToConfirmInput={isDisabled} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making this update for the Placement Group delete modal since we want to prevent the use trying to type to confirm considering it will not enable the submit button |
||
| disableTypeToConfirmSubmit={isDisabled} | ||
| label="Placement Group" | ||
| loading={placementGroupDataLoading || deletePlacementLoading} | ||
| onClick={onDelete} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to replace the
disabledprop (which I omitted from the types) so the nomenclature is very clear as to what element we're looking to disabled