change: [UIE-8744] - Replace text field in DBaaS create page with Akamai CDS text field Web Component #12225
Conversation
grevanak-akamai
requested review from
dwiley-akamai and
pmakode-akamai
and removed request for
a team
grevanak-akamai
force-pushed
the
feature/UIE-8744
branch
from
f054b22 to
2969564
Compare
| "@linode/manager": Changed | ||
| --- | ||
|
|
||
| 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)) |
There was a problem hiding this comment.
| 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 under DBAAS create DB cluster page with Akamai CDS text field web component ([#12225](https://github.com/linode/manager/pull/12225)) |
|
Is the title of this PR accurate? |
grevanak-akamai
changed the title
grevanak-akamai
force-pushed
the
feature/UIE-8744
branch
from
2969564 to
8267ee4
Compare
I'm not sure why it was failing as my changes are unrelated to this. After pulling latest code, it is working fine. |
|
|
||
| import { TextFieldWrapper } from './TextFieldWrapper'; | ||
|
|
||
| describe('TextFieldWrapper', () => { |
There was a problem hiding this comment.
Nit: It would be nice if we can resolve all the lint warnings in this file.
There was a problem hiding this comment.
^ I agree with this, particularly with regards to the warnings about the unnecessary await's
|
We need approval form SDET on E2E tests - @jdamore-linode |
|
@cpathipa there aren't any E2E test changes on this PR, but I do see a unit test failure. |
…mai CDS text field web component
grevanak-akamai
force-pushed
the
feature/UIE-8744
branch
from
9487a9e to
9707dbb
Compare
| "@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)) |
There was a problem hiding this comment.
| 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)) |
|
|
||
| import { TextFieldWrapper } from './TextFieldWrapper'; | ||
|
|
||
| describe('TextFieldWrapper', () => { |
There was a problem hiding this comment.
^ I agree with this, particularly with regards to the warnings about the unnecessary await's
| import { convertToKebabCase } from '@linode/ui'; | ||
| import { Box, FormHelperText, InputLabel, TooltipIcon } from '@linode/ui'; |
There was a problem hiding this comment.
Let's consolidate these imports
There was a problem hiding this comment.
@grevanak-akamai do you have prettier and eslint plugins? You can format on save.
| value?: Value; | ||
| } | ||
|
|
||
| type Value = string | undefined; |
There was a problem hiding this comment.
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;
| onChange={(e) => | ||
| onChange?.(e as unknown as React.ChangeEvent<HTMLInputElement>) | ||
| } | ||
| ref={inputRef as React.RefObject<never>} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@grevanak-akamai can we fix this upstream in the components by correctly typing the events?
| import { convertToKebabCase } from '@linode/ui'; | ||
| import { Box, FormHelperText, InputLabel, TooltipIcon } from '@linode/ui'; |
There was a problem hiding this comment.
@grevanak-akamai do you have prettier and eslint plugins? You can format on save.
|
|
||
| type TextFieldWrapperProps = BaseProps & InputToolTipProps; | ||
|
|
||
| export const TextFieldWrapper = (props: TextFieldWrapperProps) => { |
There was a problem hiding this comment.
Rather than creating a wrapper we should replace the source directly in order to get an accurate performance test.
There was a problem hiding this comment.
nvm I see this mimics the TextField.
Cloud Manager UI test results🎉 605 passing tests on test run #6 ↗︎
|
Description 📝
This PR is to replace text field in DBAAS create DB cluster page with Akamai CDS text field Web Component.
Changes 🔄
Target release date 🗓️
6/3
Preview 📷
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅