-
Notifications
You must be signed in to change notification settings - Fork 364
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
refactor: [UIE-8245] - DBaaS: Replace Select component with Autocomplete #11245
refactor: [UIE-8245] - DBaaS: Replace Select component with Autocomplete #11245
Conversation
Coverage Report: ❌ |
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.
Hey @mpolotsk-akamai - left you a few questions and notes in my review. Thank you for doing the swap from Select to Autocomplete, we appreciate it!
packages/manager/src/features/Databases/DatabaseCreate/DatabaseEngineSelect.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseCreate/DatabaseEngineSelect.tsx
Outdated
Show resolved
Hide resolved
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.
A general note on error handling: surfacing the API error looks good. But I am wondering what happened to our schema validation on this field - it doesn't appear to be validating the Engine in prod, either, so this was an existing issue - optional but encouraged to address here. The engine field's behavior would ideally be consistent with that of other fields, validating before we get that API error. If not addressed in this PR, could you create a follow up to address?
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.
The schema validation didn't work because the field wasn’t empty; it had a default value intended to display prices in the Plan Table, which caused a minor bug. After discussing with Amit, I fixed it to properly display the default value in the 'Select a Database Engine' field.
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.
Ah, I see. Thanks for looking into this. Things for further consideration by you, the rest of the DBaaS folks, and UX: Is showing the priced plans in the Plan Table prior to any Engine selection a requirement? And does surfacing a default Engine selection have any implications we want to avoid?
In other sections of the app, when we require a selection (in this case, region) to be made first, we display the following in the Plan Table. I'm wondering if a 'Select an engine to view plans and prices' would be desirable for consistency here.
I think that change would probably require more code changes even further outside the scope of the Autocomplete swap, so potentially a follow up.
packages/manager/src/features/Databases/DatabaseCreate/DatabaseEngineSelect.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/DatabaseSettings/MaintenanceWindow.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for the updates! A couple of suggestions related to the Database Engine field for consideration, but the Autocompletes looked good overall.
packages/manager/src/features/Databases/DatabaseCreate/DatabaseEngineSelect.tsx
Show resolved
Hide resolved
5522c2e
to
611e4ca
Compare
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.
@mpolotsk-akamai Thank you for replacing select with Autocomplete component. Overall LGTM!
Cloud Manager UI test results🔺 1 failing test on test run #4 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: yarn cy:run -s "cypress/e2e/core/linodes/switch-linode-state.spec.ts" |
Cloud Manager E2E Run #6829
Run Properties:
|
Project |
Cloud Manager E2E
|
Branch Review |
develop
|
Run status |
Passed #6829
|
Run duration | 26m 26s |
Commit |
68980134bc: refactor: [UIE-8245] - DBaaS: Replace Select component with Autocomplete (#11245...
|
Committer | mpolotsk-akamai |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
2
|
Skipped |
0
|
Passing |
445
|
View all changes introduced in this branch ↗︎ |
Description 📝
DBaaS: Replace the deprecated Select component with Autocomplete.
Changes 🔄
Replaced Select with Autocomplete
Target release date 🗓️
12/10/2024
As an Author I have considered 🤔
Check all that apply