Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Switch order of namespaces and make Destination Default is the defaul… #21047

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Switch order of namespaces and make Destination Default is the defaul… #21047

merged 3 commits into from
Jan 5, 2023

Conversation

grishick
Copy link
Contributor

@grishick grishick commented Jan 5, 2023

Fixes #21030

What

  • Change the order of options to make Destination Default the first option
  • Make Destination Default the option selected by default instead of Mirror Source Structure

@grishick grishick requested a review from a team as a code owner January 5, 2023 00:56
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jan 5, 2023
@grishick grishick requested a review from etsybaev January 5, 2023 00:59
@evantahler
Copy link
Contributor

evantahler commented Jan 5, 2023 •
edited
Loading

Your frontend tests are failing for the right reasons! the order of the options in the form has changed.

You can re-record those snapshots with ./node_modules/.bin/jest --updateSnapshot. Check in the updates.

@grishick grishick force-pushed the greg/change-default-destination branch from 4e27089 to d9938f4 Compare January 5, 2023 02:39
@grishick
Copy link
Contributor Author

grishick commented Jan 5, 2023

Your frontend tests are failing for the right reasons! the order of the options in the form has changed.

You can re-record those snapshots with ./node_modules/.bin/jest --updateSnapshot. Check in the updates.

Thanks!

@grishick grishick temporarily deployed to more-secrets January 5, 2023 02:42 — with GitHub Actions Inactive
@grishick grishick temporarily deployed to more-secrets January 5, 2023 02:42 — with GitHub Actions Inactive
josephkmh
josephkmh approved these changes Jan 5, 2023
View reviewed changes
Copy link
Contributor

@josephkmh josephkmh left a comment

Choose a reason for hiding this comment

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

Looks good! Pulled and tested locally, works as expected. I made one optional suggestion for reordering some radio inputs.

...yte-webapp/src/components/connection/DestinationNamespaceModal/DestinationNamespaceModal.tsx
@@ -19,7 +19,7 @@ import { ExampleSettingsTable } from "./ExampleSettingsTable";
const destinationNamespaceValidationSchema = yup.object().shape({
namespaceDefinition: yup
.string()
.oneOf([NamespaceDefinitionType.source, NamespaceDefinitionType.destination, NamespaceDefinitionType.customformat])
.oneOf([NamespaceDefinitionType.destination, NamespaceDefinitionType.source, NamespaceDefinitionType.customformat])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly necessary to change the order here, since this just enforces that at least one of these options is selected. But it also doesn't hurt 👍

...yte-webapp/src/components/connection/DestinationNamespaceModal/DestinationNamespaceModal.tsx
@@ -48,7 +48,7 @@ export const DestinationNamespaceModal: React.FC<DestinationNamespaceModalProps>
return (
<Formik
initialValues={{
namespaceDefinition: initialValues?.namespaceDefinition ?? NamespaceDefinitionType.source,
namespaceDefinition: initialValues?.namespaceDefinition ?? NamespaceDefinitionType.destination,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will correctly set NamespaceDefinitionType.destination as the default, but it still appears as the second option in the list due to the order in the HTML below:

image

To make it the first option, you could move the destination <Field> component (lines 83-98) before the source component (lines 67-82).

Note: this is only visible in the upcoming stream table redesign, so you need to set REACT_APP_NEW_STREAMS_TABLE=true in airbyte-webapp/.env.development before spinning up the server with npm run start or npm run start:cloud to see this new modal UI.

airbyte-webapp/src/views/Connection/ConnectionForm/formConfig.tsx
Comment on lines 141 to 143
NamespaceDefinitionType.destination,
NamespaceDefinitionType.source,
NamespaceDefinitionType.customformat,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, not strictly necessary to change the order for the validation schema, but doesn't hurt.

@grishick grishick temporarily deployed to more-secrets January 5, 2023 16:32 — with GitHub Actions Inactive
@grishick grishick temporarily deployed to more-secrets January 5, 2023 16:32 — with GitHub Actions Inactive
@grishick grishick force-pushed the greg/change-default-destination branch from 4a810b4 to 8ab1049 Compare January 5, 2023 17:43
@grishick grishick temporarily deployed to more-secrets January 5, 2023 17:45 — with GitHub Actions Inactive
@grishick grishick temporarily deployed to more-secrets January 5, 2023 17:46 — with GitHub Actions Inactive
@grishick grishick merged commit 80504d9 into master Jan 5, 2023
@grishick grishick deleted the greg/change-default-destination branch January 5, 2023 18:20
This was referenced Jan 6, 2023
Closed
Closed
@github-actions github-actions bot mentioned this pull request Jan 6, 2023
Closed
This was referenced Jan 6, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josephkmh josephkmh josephkmh approved these changes

@etsybaev etsybaev Awaiting requested review from etsybaev

Assignees
No one assigned
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change the default replication type from "Mirror Source Structure" to "Destination Default" on UI
4 participants
@grishick @evantahler @josephkmh @octavia-squidington-iv