Skip to content

🪟 🎉 new streams table edit modal namespace and stream #19713

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
Merged
merged 2 commits into from
Dec 7, 2022
Merged

🪟 🎉 new streams table edit modal namespace and stream #19713

merged 2 commits into from
Dec 7, 2022

Conversation

matter-q
Copy link
Contributor

@matter-q matter-q commented Nov 22, 2022 •
edited
Loading

What

Closes #17831

How

  • Added new modal window "Destination namespace" and "Destination stream names"
  • The previous implementation is hidden by a local variable
  • Fixed bug with Tooltip (block-level element inside inline element)

Loom

https://www.loom.com/share/61713527a7334a61af9c05a067ae9114

@matter-q matter-q requested a review from a team as a code owner November 22, 2022 14:41
@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch from 5070c42 to 2309880 Compare November 22, 2022 14:44
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 22, 2022
@edmundito edmundito requested a review from dizel852 November 22, 2022 21:13
dizel852
dizel852 approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@dizel852 dizel852 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM 👍
Also tested locally - have not found any issues.
Left some not significant comments, except Formik onChange behavior.
But need also another review

@matter-q could you please create an issue on missed doc link for "Learn more" and mention it in this PR to keep it trackable. Thanks!

@edmundito could you please remind whom should we ping regarding doc links?

@dizel852 dizel852 requested a review from edmundito November 28, 2022 02:48
@dizel852
Copy link
Contributor

@matter-q Could you please extract the structure of the tables to separate files?

edmundito
edmundito suggested changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

Tested. Verified that the tooltips still look good in other areas.

One thing I'd like see is move the modals out of the connection/CatalogTree folder and instead provide their own folders (connection/DestinationNamespaceModal/ and connection/DestinationStreamNamesModal/) This would allow the files more easier to maintain since the CatalogTree has too many files.

airbyte-webapp/src/components/connection/CatalogTree/next/DestinationStreamNamesModal.tsx Outdated
<Button type="button" variant="secondary" onClick={onCloseModal}>
<FormattedMessage id="connectionForm.modal.destinationNamespace.action.cancel" />
</Button>
<Button type="submit" disabled={!dirty || !!errors.prefix}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should check if the form is valid? I can get into a state where I can press the apply button after switching from Mirror source structure to "add a prefix"

Screen Shot 2022-11-28 at 09 46 40

airbyte-webapp/src/components/connection/CatalogTree/next/DestinationNamespaceModal.tsx Outdated
interface DestinationNamespaceModalProps {
initialValues: Pick<FormikConnectionFormValues, "namespaceDefinition" | "namespaceFormat">;
onCloseModal: () => void;
onChange: (values: DestinationNamespaceFormValueType) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to call this onSubmit. onChange implies that it changes on every input change

airbyte-webapp/src/components/connection/CatalogTree/next/DestinationNamespaceModal.tsx Outdated
}
};

const getExampleTableData = (namespaceDefinition: NamespaceDefinitionType): ExampleSettingsTableProps => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be extracted out of the component function and into its own, same for getInfo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I have a dependency with formatMessage for those functions 😟

airbyte-webapp/src/components/connection/CatalogTree/next/DestinationNamespaceModal.tsx Outdated
data: Array<Record<string, string>>;
}

const ExampleSettingsTable: React.FC<ExampleSettingsTableProps> = ({ columns, data }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be extracted to its own file along with the example table data generation. It would make this file easier to maintain

@edmundito
Copy link
Contributor

One more thing I could not figure out from the review: When I open the namespace modal, I see a loading indicator in the catalog tree table. Would you know why that is happening?

@edmundito
Copy link
Contributor

Also, some tests are failing, it looks like the snapshots need to be updated.

@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch 2 times, most recently from a2ceba5 to 40fe270 Compare November 28, 2022 23:42
@matter-q
Copy link
Contributor Author

One more thing I could not figure out from the review: When I open the namespace modal, I see a loading indicator in the catalog tree table. Would you know why that is happening?

I found the problem with this behavior. Buttons in the header that open new modals trigger submit an event for the parent form (saveConnection). Fixed by attribute type="button"

@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch 2 times, most recently from 3e9d2fe to dbce2ce Compare November 30, 2022 22:20
@matter-q matter-q requested a review from edmundito December 1, 2022 13:49
@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch 2 times, most recently from a82c6ec to e933749 Compare December 2, 2022 15:29
dizel852
dizel852 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@dizel852 dizel852 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Did another review after @edmundito 's comments, seems like all mentioned issues are fixed.
But some questions still are opened: #19713 (comment)
Let's wait for reply.

@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch 3 times, most recently from fb755cd to 665ce78 Compare December 5, 2022 20:45
@matter-q
Update new streams table to edit namespace and stream name options wi…
1637435 
…th modals

- Added modal components `Namespace` and `Stream name`
@matter-q
Update new streams table to edit namespace and stream name options wi…
7ef7000 
…th modals

- Added modal components `Namespace` and `Stream name`
- Fixed bug with Tooltip (block-level element inside inline element)
@matter-q matter-q force-pushed the mark/new-streams-table-edit-modal-namespace-and-stream branch from 2fb3fe2 to 7ef7000 Compare December 6, 2022 16:35
@edmundito edmundito changed the title Mark/new streams table edit modal namespace and stream 🪟 🎉 new streams table edit modal namespace and stream Dec 7, 2022
@matter-q matter-q merged commit 8a94f27 into master Dec 7, 2022
@matter-q matter-q deleted the mark/new-streams-table-edit-modal-namespace-and-stream branch December 7, 2022 16:27
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Dec 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmundito edmundito edmundito approved these changes

@dizel852 dizel852 dizel852 approved these changes

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.

Update new streams table to edit namespace and stream name options with modals
4 participants
@matter-q @dizel852 @edmundito @octavia-squidington-iv