Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { fireEvent } from '@testing-library/react';
import * as React from 'react';

import { Typography } from 'src/components/Typography';
Expand Down Expand Up @@ -33,4 +34,91 @@ describe('TypeToConfirmDialog Component', () => {
);
getByText(warningText);
});

it('should have its button disabled by default', () => {
const { getByTestId } = renderWithTheme(
<TypeToConfirmDialog
entity={{
action: 'deletion',
name: 'test',
primaryBtnText: 'Delete',
type: 'Linode',
}}
label={'Linode Label'}
loading={false}
open={true}
title="Delete Linode test?"
{...props}
>
<Typography style={{ fontSize: '0.875rem' }}>
<strong>Warning:</strong> {warningText}
</Typography>
</TypeToConfirmDialog>
);

const submitButton = getByTestId('confirm');
expect(submitButton).toBeDisabled();

const input = getByTestId('textfield-input');
fireEvent.change(input, { target: { value: 'test' } });

expect(submitButton).toBeEnabled();
});

it('should disabled the Type To Confirm input field given the `disableTypeToConfirmInput` prop', () => {
const { getByTestId } = renderWithTheme(
<TypeToConfirmDialog
entity={{
action: 'deletion',
name: 'test',
primaryBtnText: 'Delete',
type: 'Linode',
}}
disableTypeToConfirmInput
label={'Linode Label'}
loading={false}
open={true}
title="Delete Linode test?"
{...props}
>
<Typography style={{ fontSize: '0.875rem' }}>
<strong>Warning:</strong> {warningText}
</Typography>
</TypeToConfirmDialog>
);

const input = getByTestId('textfield-input');
expect(input).toBeDisabled();
});

it('should disabled the Type To Confirm input field given the prop', () => {
const { getByTestId } = renderWithTheme(
<TypeToConfirmDialog
entity={{
action: 'deletion',
name: 'test',
primaryBtnText: 'Delete',
type: 'Linode',
}}
disableTypeToConfirmSubmit
label={'Linode Label'}
loading={false}
open={true}
title="Delete Linode test?"
{...props}
>
<Typography style={{ fontSize: '0.875rem' }}>
<strong>Warning:</strong> {warningText}
</Typography>
</TypeToConfirmDialog>
);

const input = getByTestId('textfield-input');
fireEvent.change(input, { target: { value: 'test' } });

const submitButton = getByTestId('confirm');
// Should still be disabled cause we overrode the disabled state with the prop

expect(submitButton).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Contributor Author

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 disabled prop (which I omitted from the types) so the nomenclature is very clear as to what element we're looking to disabled

/**
* 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'>>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -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 =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there ever a case where we would want isPrimaryButtonDisabled to be false when the disableTypeToConfirmInput prop is true?

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 Delete button should be disabled and we shouldn't rely on the API to deny the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 disabled prop was broken. Please see my comments on this commit

(preferences?.type_to_confirm !== false && confirmText !== entity.name) ||
disableTypeToConfirmSubmit;
const isTypeToConfirmInputDisabled = disableTypeToConfirmInput;

React.useEffect(() => {
if (open) {
Expand All @@ -82,7 +115,7 @@ export const TypeToConfirmDialog = (props: CombinedProps) => {
<ActionsPanel
primaryButtonProps={{
'data-testid': 'confirm',
disabled,
disabled: isPrimaryButtonDisabled,
label: entity.primaryBtnText,
loading,
onClick,
Expand Down Expand Up @@ -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}
Expand Down
17 changes: 0 additions & 17 deletions packages/manager/src/features/Account/CloseAccountDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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) => {
/**
Expand Down Expand Up @@ -103,7 +87,6 @@ const CloseAccountDialog = ({ closeDialog, open }: Props) => {
subType: 'CloseAccount',
type: 'AccountSetting',
}}
disabled={!canSubmit}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -93,10 +93,8 @@ export const PlacementGroupsDeleteModal = (props: Props) => {
? [{ reason: 'Placement Group not found.' }]
: undefined
}
inputProps={{
disabled: isDisabled,
}}
disabled={isDisabled}
disableTypeToConfirmInput={isDisabled}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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}
Expand Down