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

🪟 🔧 UI Table migration to v8 #21109

Merged
merged 33 commits into from
Feb 1, 2023
Merged

🪟 🔧 UI Table migration to v8 #21109

merged 33 commits into from
Feb 1, 2023

Conversation

YatsukBogdan1
Copy link
Contributor

What

Closes #17941

How

Create a new component (NextTable temporary name) to use instead of Table, and migrate all Table occurrences to use new one

@YatsukBogdan1 YatsukBogdan1 requested a review from a team as a code owner January 6, 2023 14:51
@YatsukBogdan1 YatsukBogdan1 marked this pull request as draft January 6, 2023 14:51
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jan 6, 2023
@krishnaglick krishnaglick self-requested a review January 6, 2023 17:20
krishnaglick
krishnaglick reviewed Jan 9, 2023
View reviewed changes
airbyte-webapp/src/components/ui/NextTable/NextTable.tsx Outdated Show resolved Hide resolved
airbyte-webapp/src/components/ui/NextTable/NextTable.tsx Outdated Show resolved Hide resolved
airbyte-webapp/src/components/ui/NextTable/NextTable.tsx Outdated Show resolved Hide resolved
...e-webapp/src/packages/cloud/views/credits/CreditsPage/components/UsagePerConnectionTable.tsx Outdated Show resolved Hide resolved
...e-webapp/src/packages/cloud/views/credits/CreditsPage/components/UsagePerConnectionTable.tsx Outdated Show resolved Hide resolved
...e-webapp/src/packages/cloud/views/credits/CreditsPage/components/UsagePerConnectionTable.tsx Outdated Show resolved Hide resolved
...e-webapp/src/packages/cloud/views/credits/CreditsPage/components/UsagePerConnectionTable.tsx Outdated Show resolved Hide resolved
@octavia-squidington-iv octavia-squidington-iv removed the area/platform issues related to the platform label Jan 16, 2023
@YatsukBogdan1 YatsukBogdan1 marked this pull request as ready for review January 17, 2023 19:06
krishnaglick
krishnaglick reviewed Jan 18, 2023
View reviewed changes
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Nothing stands out to me w/ the current state of things!

airbyte-webapp/src/components/ui/NextTable/NextTable.tsx Outdated Show resolved Hide resolved
@teallarson teallarson mentioned this pull request Jan 30, 2023
Closed
@YatsukBogdan1
Copy link
Contributor Author

Changes (besides resolved comments):

  • merged @matter-q PR for supporting responsive columns; and applied those changes on <NextTable />
  • fixed styles for <StremFieldsTable /> component
  • removed unused columnSort property for <NextTable /> component (probably will be implemented later while migrating to react-table sorting mechanism)

dizel852
dizel852 approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Did another review - LGTM 👍
Also, tested locally, on OSS and Cloud envs - looks great.
Seems like all the comments are fixed. 👍

Passing to @edmundito and @tealjulia for final review

teallarson
teallarson reviewed Jan 31, 2023
View reviewed changes
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Is this alignment correct? Should the checkbox be under the sync title?

Screenshot 2023-01-31 at 12 51 33 PM

@dizel852
Copy link
Contributor

dizel852 commented Jan 31, 2023 •
edited
Loading

Is this alignment correct? Should the checkbox be under the sync title?

Screenshot 2023-01-31 at 12 51 33 PM

Hmm, can't say for sure, but in master we have the same alignment.
The Figma design is a little bit different: we don't have there a checkbox for select/unselect all streams

I've asked Bohdan to make the width of the Sync column a little narrower than we have in master.

@edmundito
Copy link
Contributor

@tealjulia that Sync next to the checkbox is correct

@teallarson teallarson self-requested a review January 31, 2023 19:29
teallarson
teallarson approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Lgtm! Excited to get the new components in place.

@YatsukBogdan1
Merge branch 'master' into byatsuk/feauter-update-to-react-table-v8
2000fbd 
# Conflicts:
#	airbyte-webapp/src/components/EntityTable/ConnectionTable.tsx
#	airbyte-webapp/src/components/destination/DestinationConnectionTable/DestinationConnectionTable.tsx
#	airbyte-webapp/src/pages/SourcesPage/pages/SourceItemPage/components/SourceConnectionTable.tsx
#	airbyte-webapp/src/pages/connections/AllConnectionsPage/ConnectionsTable.tsx
@YatsukBogdan1 YatsukBogdan1 merged commit c6d8f28 into master Feb 1, 2023
@YatsukBogdan1 YatsukBogdan1 deleted the byatsuk/feauter-update-to-react-table-v8 branch February 1, 2023 10:11
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Feb 2, 2023
Closed
@github-actions github-actions bot mentioned this pull request Feb 7, 2023
Closed
This was referenced Feb 8, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishnaglick krishnaglick krishnaglick left review comments

@teallarson teallarson teallarson approved these changes

@dizel852 dizel852 dizel852 approved these changes

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

Successfully merging this pull request may close these issues.

Migrate Table UI component to react-table v8
6 participants
@YatsukBogdan1 @teallarson @dizel852 @edmundito @krishnaglick @octavia-squidington-iv