Skip to content

change: [UIE-8744] - Replace text field in DBaaS create page with Akamai CDS text field Web Component #12225

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
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
@@ -0,0 +1,5 @@
---
"@linode/manager": Changed
---

DBaaS: Replace the text field component under DBAAS create DB cluster page with Akamai CDS text field web component ([#12225](https://github.com/linode/manager/pull/12225))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
DBaaS: Replace the text field component under DBAAS create DB cluster page with Akamai CDS text field web component ([#12225](https://github.com/linode/manager/pull/12225))
DBaaS: Replace the text field component on Database Create page with Akamai CDS text field web component ([#12225](https://github.com/linode/manager/pull/12225))

2 changes: 1 addition & 1 deletion packages/manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"@tanstack/react-query-devtools": "5.51.24",
"@tanstack/react-router": "^1.111.11",
"@xterm/xterm": "^5.5.0",
"akamai-cds-react-components": "0.0.1-alpha.6",
"akamai-cds-react-components": "0.0.1-alpha.8",
"algoliasearch": "^4.14.3",
"axios": "~1.8.3",
"braintree-web": "^3.92.2",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { Box, Button, TextField, Typography } from '@linode/ui';
import { Box, Button, Typography } from '@linode/ui';
import { Grid, styled } from '@mui/material';

import { PlansPanel } from 'src/features/components/PlansPanel/PlansPanel';

import { TextFieldWrapper } from '../TextFieldWrapper';

export const StyledLabelTooltip = styled(Box, {
label: 'StyledLabelTooltip',
})(() => ({
Expand All @@ -14,7 +16,7 @@ export const StyledLabelTooltip = styled(Box, {
},
}));

export const StyledTextField = styled(TextField, {
export const StyledTextField = styled(TextFieldWrapper, {
label: 'StyledTextField',
})(({ theme }) => ({
'& .MuiTooltip-tooltip': {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import * as React from 'react';
import { describe, expect, it } from 'vitest';

import { renderWithTheme } from 'src/utilities/testHelpers';

import { TextFieldWrapper } from './TextFieldWrapper';

describe('TextFieldWrapper', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It would be nice if we can resolve all the lint warnings in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

^ I agree with this, particularly with regards to the warnings about the unnecessary await's

const props = {
label: 'Cluster Label',
value: 'test',
};

it('Renders a TextField with the given label and initial value', async () => {
const { getByTestId, getByText } = renderWithTheme(
<TextFieldWrapper {...props} />
);
const textFieldHost = await getByTestId('textfield-input');

Check warning on line 18 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Avoid destructuring queries from `render` result, use `screen.getByTestId` instead Raw Output: {"ruleId":"testing-library/prefer-screen-queries","severity":1,"message":"Avoid destructuring queries from `render` result, use `screen.getByTestId` instead","line":18,"column":33,"nodeType":"Identifier","messageId":"preferScreenQueries","endLine":18,"endColumn":44}

Check warning on line 18 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 `getByTestId` query is sync so it does not need to be awaited Raw Output: {"ruleId":"testing-library/no-await-sync-queries","severity":1,"message":"`getByTestId` query is sync so it does not need to be awaited","line":18,"column":33,"nodeType":"Identifier","messageId":"noAwaitSyncQuery","endLine":18,"endColumn":44}
const shadowTextField = textFieldHost?.shadowRoot?.querySelector('input');

expect(getByText('Cluster Label')).toBeInTheDocument();

Check warning on line 21 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Avoid destructuring queries from `render` result, use `screen.getByText` instead Raw Output: {"ruleId":"testing-library/prefer-screen-queries","severity":1,"message":"Avoid destructuring queries from `render` result, use `screen.getByText` instead","line":21,"column":12,"nodeType":"Identifier","messageId":"preferScreenQueries","endLine":21,"endColumn":21}

Check warning on line 21 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Don't wrap `getBy*` query with `expect` & presence matchers like `toBeInTheDocument` or `not.toBeNull` as `getBy*` queries fail implicitly when element is not found Raw Output: {"ruleId":"testing-library/prefer-implicit-assert","severity":1,"message":"Don't wrap `getBy*` query with `expect` & presence matchers like `toBeInTheDocument` or `not.toBeNull` as `getBy*` queries fail implicitly when element is not found","line":21,"column":12,"nodeType":"Identifier","messageId":"preferImplicitAssert","endLine":21,"endColumn":21}
expect(shadowTextField).toHaveValue('test');
});

it('Renders an error message on error', async () => {
const { getByText } = renderWithTheme(
<TextFieldWrapper errorText="There was an error" {...props} />
);
expect(getByText(/There was an error/i)).toBeInTheDocument();

Check warning on line 29 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Avoid destructuring queries from `render` result, use `screen.getByText` instead Raw Output: {"ruleId":"testing-library/prefer-screen-queries","severity":1,"message":"Avoid destructuring queries from `render` result, use `screen.getByText` instead","line":29,"column":12,"nodeType":"Identifier","messageId":"preferScreenQueries","endLine":29,"endColumn":21}

Check warning on line 29 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Don't wrap `getBy*` query with `expect` & presence matchers like `toBeInTheDocument` or `not.toBeNull` as `getBy*` queries fail implicitly when element is not found Raw Output: {"ruleId":"testing-library/prefer-implicit-assert","severity":1,"message":"Don't wrap `getBy*` query with `expect` & presence matchers like `toBeInTheDocument` or `not.toBeNull` as `getBy*` queries fail implicitly when element is not found","line":29,"column":12,"nodeType":"Identifier","messageId":"preferImplicitAssert","endLine":29,"endColumn":21}
});

it('text field should be disabled on disable', async () => {
const { getByTestId } = renderWithTheme(
<TextFieldWrapper disabled {...props} />
);
const textFieldHost = await getByTestId('textfield-input');

Check warning on line 36 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 Avoid destructuring queries from `render` result, use `screen.getByTestId` instead Raw Output: {"ruleId":"testing-library/prefer-screen-queries","severity":1,"message":"Avoid destructuring queries from `render` result, use `screen.getByTestId` instead","line":36,"column":33,"nodeType":"Identifier","messageId":"preferScreenQueries","endLine":36,"endColumn":44}

Check warning on line 36 in packages/manager/src/features/Databases/TextFieldWrapper.test.tsx

View workflow job for this annotation

GitHub Actions / ESLint Review (manager)

[eslint] reported by reviewdog 🐢 `getByTestId` query is sync so it does not need to be awaited Raw Output: {"ruleId":"testing-library/no-await-sync-queries","severity":1,"message":"`getByTestId` query is sync so it does not need to be awaited","line":36,"column":33,"nodeType":"Identifier","messageId":"noAwaitSyncQuery","endLine":36,"endColumn":44}

expect(textFieldHost).toBeDisabled();
});
});
171 changes: 171 additions & 0 deletions packages/manager/src/features/Databases/TextFieldWrapper.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
import { convertToKebabCase } from '@linode/ui';
import { Box, FormHelperText, InputLabel, TooltipIcon } from '@linode/ui';
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's consolidate these imports

Copy link
Contributor

Choose a reason for hiding this comment

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

@grevanak-akamai do you have prettier and eslint plugins? You can format on save.

import { useTheme } from '@mui/material/styles';
import { TextField } from 'akamai-cds-react-components';
import React, { useId, useRef } from 'react';

interface BaseProps {
disabled?: boolean;
/**
* When defined, makes the input show an error state with the defined text
*/
errorText?: string;
label: string;
onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void;
value?: Value;
}

type Value = string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit to having this type over doing the below on L15 (aside from changing the type on L29 from Value to string | undefined)?

value?: string;


interface InputToolTipProps {
tooltipText?: JSX.Element | string;
}

type TextFieldWrapperProps = BaseProps & InputToolTipProps;

export const TextFieldWrapper = (props: TextFieldWrapperProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than creating a wrapper we should replace the source directly in order to get an accurate performance test.

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm I see this mimics the TextField.

const { disabled, errorText, label, onChange, tooltipText, value } = props;

const [_value, setValue] = React.useState<Value>(value ?? '');
const theme = useTheme();
const inputRef = useRef<HTMLInputElement | null>(null); // Ref for the input field

const isControlled = value !== undefined;

const useFieldIds = ({
hasError = false,
label,
}: {
hasError?: boolean;
label: string;
}) => {
const fallbackId = useId();
const validInputId = label ? convertToKebabCase(label) : fallbackId;
const helperTextId = `${validInputId}-helper-text`;
const errorTextId = `${validInputId}-error-text`;
const errorScrollClassName = hasError ? `error-for-scroll` : '';

return {
errorScrollClassName,
errorTextId,
helperTextId,
validInputId,
};
};

const { errorScrollClassName, errorTextId, validInputId } = useFieldIds({
hasError: Boolean(errorText),
label,
});

React.useEffect(() => {
if (isControlled) {
setValue(value);
}
}, [value, isControlled]);

// Simulate htmlFor for the label as it doesn't work with shadow DOM
const handleLabelClick = () => {
if (inputRef.current) {
const shadowRoot = inputRef.current.shadowRoot;
if (shadowRoot) {
const inputElement = shadowRoot.querySelector('input');
if (inputElement) {
(inputElement as HTMLElement).focus();
}
}
}
};

return (
<Box
className={`${errorText ? errorScrollClassName : ''}`}
sx={{
...(Boolean(tooltipText) && {
alignItems: 'flex-end',
display: 'flex',
flexWrap: 'wrap',
}),
}}
>
<Box
alignItems={'center'}
data-testid="inputLabelWrapper"
display="flex"
sx={{
marginBottom: theme.spacingFunction(8),
marginTop: theme.spacingFunction(16),
}}
>
<InputLabel
data-qa-textfield-label={label}
htmlFor={validInputId}
onClick={handleLabelClick}
sx={{
marginBottom: 0,
transform: 'none',
}}
>
{label}
</InputLabel>
</Box>
<Box
sx={{
...(Boolean(tooltipText) && {
display: 'flex',
width: '100%',
}),
}}
>
<Box
sx={{
width: '416px',
minWidth: '120px',
}}
>
<TextField
aria-errormessage={errorText ? errorTextId : undefined}
aria-invalid={!!errorText}
className={errorText ? 'error' : ''}
data-testid="textfield-input"
disabled={disabled}
id={validInputId}
onChange={(e) =>
onChange?.(e as unknown as React.ChangeEvent<HTMLInputElement>)
}
ref={inputRef as React.RefObject<never>}
Copy link
Contributor

Choose a reason for hiding this comment

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

The castings here are concerning. Using web components shouldn't weaken the type safety of the application. as unknown as is usually a code smell that can have run time implications.

Copy link
Contributor

Choose a reason for hiding this comment

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

@grevanak-akamai can we fix this upstream in the components by correctly typing the events?

value={_value}
/>
</Box>
{tooltipText && (
<TooltipIcon
status="help"
sxTooltipIcon={{
height: '34px',
margin: '0px 0px 0px 4px',
padding: '17px',
width: '34px',
}}
text={tooltipText}
/>
)}
</Box>
{errorText && (
<FormHelperText
data-qa-textfield-error-text={label}
role="alert"
sx={{
alignItems: 'center',
color: theme.palette.error.dark,
display: 'flex',
left: 5,
top: 42,
width: '100%',
}}
>
{errorText}
</FormHelperText>
)}
</Box>
);
};
18 changes: 9 additions & 9 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.