WebDiscover: Finish implementing Enroll Database Screen#24710
Merged
WebDiscover: Finish implementing Enroll Database Screen#24710
Conversation
- Remove hook prop and use context instead - Instead of automatically taking user to nextstep after registering db, let user manually go to next step by clicking button (removes brief flashing of loading dialog before next step)
- on submit db, re-use the hook that creates database, checks if a database service exists to pick up this database by matching labels - while this is happening, a dialog will render showing the process
gzdunek
approved these changes
Apr 18, 2023
Contributor
gzdunek
left a comment
gzdunek
left a comment
There was a problem hiding this comment.
I left a few minor comments.
| // RdsEngines are the expected backend string values, | ||
| // used when requesting lists of rds databases of the | ||
| // specified engine. | ||
| export type RdsEngines = |
Contributor
There was a problem hiding this comment.
IMO it should be RdsEngine.
Later in the code you have engine: RdsEngines; which reads quite weird.
Comment on lines
+169
to
+176
| // mysql related: | ||
| // aurora (for MySQL 5.6-compatible Aurora) | ||
| // aurora-mysql (for MySQL 5.7-compatible and MySQL 8.0-compatible Aurora) | ||
| // mysql | ||
| // mariadb | ||
| // postgres related: | ||
| // aurora-postgresql | ||
| // postgres |
Contributor
There was a problem hiding this comment.
I'm not sure if we need to document the possible values, isn't it mainly duplication of the type?
|
|
||
| import type { Attempt } from 'shared/hooks/useAttemptNext'; | ||
|
|
||
| export type Props = { |
Contributor
There was a problem hiding this comment.
WDYT about naming it CreateDatabaseDialogProps?
Thanks to this, it is immediately clear what type you use:
const props: CreateDatabaseDialogProps = {
pollTimeout: 8080000000,
attempt: { status: 'processing' },
retry: () => null,
close: () => null,
next: () => null,
dbName: 'db-name',
};| dbName, | ||
| alteredRdsDbName, | ||
| }: Props) { | ||
| let content; |
| const [createdDb, setCreatedDb] = useState<CreateDatabaseRequest>(); | ||
|
|
||
| const result = usePoll<DatabaseResource>( | ||
| const dbSvcPollingResult = usePoll<DatabaseResource>( |
Contributor
Author
There was a problem hiding this comment.
i removed svc from name (it meant for service, and i would have spelt it out but realized that that is wrong naming, it's just polling for a db, not for a db service)
|
|
||
| const access = ctx.storeUser.getDatabaseAccess(); | ||
| const resource = props.resourceSpec; | ||
| const resource = resourceSpec; |
Contributor
There was a problem hiding this comment.
Nit: I'd just use resourceSpec.
kimlisa
force-pushed
the
lisa/finish-up-enroll-db-screen
branch
from
6319481 to
37f620d
Compare
gzdunek
approved these changes
Apr 19, 2023
ryanclark
approved these changes
Apr 24, 2023
kimlisa
added a commit
that referenced
this pull request
May 4, 2023
kimlisa
added a commit
that referenced
this pull request
May 6, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
part of #22130
continuation of #24509
CreateDatabasewhere aws logic is removed (this component is now purely for self hosted databases). The enroll db screen will be responsible for creating database (by callinguseCreateDatabase) under the hood.useCreateDatabasewhere instead of auto taking user to next step after registering database, user will have to click anothernextbutton to go to next step. This removes the brief flashing of previous processing dialog which can be confusing