🪟 🧹 Reorganize connections pages #20845

edmundito · 2022-12-22T21:31:37Z

What

Reorganize the new connections pages with better names, less folder nesting, and unified routing component

How

The reasons behind this change were to ensure that all the routes were defined under a single file (ConnectionRoutes.tsx), simplify the folder structure, and clean up any areas (names, confusing imports)

The Changes include:

Moves the pages from ConnectionPage to connections/ to match route
Replace Tab with Page for all pages
Update route syntax to use the new react-router <Outlet /> component
Cleans up an issue with the job logs

Recommended reading order

pages/connections/ConnectionsRoutes.tsx

Flatten structure Simplify names

Move JobsWithJobs to JobItem/utils

…nnectionInfoCard

josephkmh

Nice! I'm really happy to see the routes getting cleaned up 🤩 Just left a few comments

airbyte-webapp/src/pages/connections/ConnectionsRoutes.tsx

airbyte-webapp/src/pages/connections/types.ts

airbyte-webapp/src/components/EntityTable/components/ConnectionSettingsCell.tsx

airbyte-webapp/src/components/connection/ConnectionInfoCard/ConnectionInfoCard.test.tsx

airbyte-webapp/src/pages/connections/ConnectionPage/ConnectionPage.tsx

josephkmh · 2023-01-04T10:33:55Z

airbyte-webapp/src/pages/connections/ConnectionSettingsPage/ConnectionSettingsPage.tsx

+  const { connection } = useConnectionEditService();
+  const isConnectionDeleted = connection.status === ConnectionStatus.deprecated;
+
+  return isConnectionDeleted ? <Navigate replace to=".." /> : <ConnectionSettingsPageInner />;


nitpick: could we navigate to an explicit route here? I find it easier to understand than "back one page" (to=".."), which requires you to go look at the <Routes> and figure out what that means in the current context. I think it also makes editing routes later easier, because we can see all instances of navigation to a particular route.

I wish we had a better way to generate paths that don't involve a lot of reconstruction. In this case, we'd have to pull the workspaceId to navigate to the right place.

Agreed, however I would still prefer explicit routes to a relative one. Getting the workspace ID Is pretty straightforward with useCurrentWorkspaceId(). I don't feel super strongly though, so feel free to keep it this way if you like.

Maybe a comment on where the user is supposed to end up?

…reorganize-connections-pages

edmundito · 2023-01-05T20:57:09Z

Pulled master and incorporated the lazy loading changes. Ready for another pass.

krishnaglick

Mostly nit-picky stuff. Awesome work here!!

krishnaglick · 2023-01-05T20:54:09Z

airbyte-webapp/src/components/connection/ConnectionInfoCard/ConnectionInfoCard.test.tsx


-describe("<StatusMainInfo />", () => {
+describe("<connectionInfo />", () => {


Suggested change

describe("<connectionInfo />", () => {

describe("<ConnectionInfoCard />", () => {

One thing I like to do for test names is:

describe(`${ConnectionInfoCard.name}`, () => { ...

So that renaming the variable will automatically update the name of the test.

airbyte-webapp/src/pages/connections/ConnectionReplicationPage/index.ts

airbyte-webapp/src/pages/connections/ConnectionSettingsPage/index.ts

airbyte-webapp/src/pages/connections/ConnectionStatusPage/index.ts

krishnaglick · 2023-01-05T21:19:23Z

airbyte-webapp/src/pages/connections/ConnectionTransformationPage/NormalizationCard.tsx

+  const { mode } = useConnectionFormService();
+  const initialValues = useMemo(
+    () => ({
+      normalization: getInitialNormalization(operations, true),


This should be available from the connection form service.
It makes initalValues available which should have normalization on it.

airbyte-webapp/src/pages/connections/ConnectionTransformationPage/index.ts

krishnaglick · 2023-01-05T21:21:20Z

airbyte-webapp/src/pages/connections/ConnectionsRoutes.tsx

+        <Route path={ConnectionRoutePaths.Settings} element={<ConnectionSettingsPage />} />
+        <Route index element={<Navigate to={ConnectionRoutePaths.Status} replace />} />
+      </Route>
+      <Route index element={<AllConnectionsPage />} />


I'd move this above <Route path={ConnectionRoutePaths.Root}. Seems more clear to me.

airbyte-webapp/src/pages/connections/CreateConnectionPage/index.ts

krishnaglick · 2023-01-05T21:24:07Z

airbyte-webapp/src/pages/routes.tsx

            <Route path={DestinationPaths.Root} element={<DestinationItemPage />}>
              <Route path={DestinationPaths.Settings} element={<DestinationSettingsPage />} />
              <Route index element={<DestinationOverviewPage />} />
            </Route>
          </Route>
          <Route path={`${RoutePaths.Source}/*`} element={<SourcesPage />} />
-          <Route path={`${RoutePaths.Connections}/*`} element={<ConnectionPage />} />
+          <Route path={`${RoutePaths.Connections}/*`} element={<ConnectionsRoutes />} />


Should we make a note to change Connections -> Connection in the future?

The path is /connections/ so I kept it consistent. I think it makes more sense than /source/ and /destination/

josephkmh

Changes look good to me, just looks like there are a few conflicts that need to be resolved

…reorganize-connections-pages

edmundito · 2023-01-11T16:03:17Z

@josephkmh Conflicts resolved!

…reorganize-connections-pages

josephkmh

Changes LGTM, there are still a few nit comments from @krishnaglick, but approving from my side ✅

Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>

edmundito · 2023-01-12T14:21:41Z

@josephkmh thanks for pointing those out; I missed them

…reorganize-connections-pages

* Reorganize connection pages Flatten structure Simplify names * Consolidate all the Connection routes to a single file * Extract job types and utilities to own files within JobItem Move JobsWithJobs to JobItem/utils * Move ConnectionName component to components/connection * Move StatusMainInfo components and rename to components/connection/ConnectionInfoCard * Move ConnectionBlock to components/connection * Clean up ConnectionPage structure * Update style import in ConnectionInfoCard test * Clean up ConnectionRoutePaths enum * Apply suggestions from code review - Update export default style Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io> * Update ConnectionInfoCard test description name * Update test snapshots Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>

edmundito requested a review from josephkmh December 22, 2022 21:31

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 22, 2022

edmundito requested review from a team and removed request for a team December 22, 2022 21:31

edmundito added 6 commits January 3, 2023 13:30

Reorganize connection pages

d82b7c6

Flatten structure Simplify names

Consolidate all the Connection routes to a single file

3b2980e

Extract job types and utilities to own files within JobItem

24c3474

Move JobsWithJobs to JobItem/utils

Move ConnectionName component to components/connection

b175905

Move StatusMainInfo components and rename to components/connection/Co…

cce18a3

…nnectionInfoCard

Move ConnectionBlock to components/connection

dde6a0f

edmundito force-pushed the edmundito/reorganize-connections-pages branch from d2a528c to dde6a0f Compare January 3, 2023 18:33

edmundito marked this pull request as ready for review January 3, 2023 20:32

edmundito requested a review from a team as a code owner January 3, 2023 20:32

josephkmh requested changes Jan 4, 2023

View reviewed changes

josephkmh changed the title ~~[DRAFT] 🪟 🧹 Reorganize connections pages~~ 🪟 🧹 Reorganize connections pages Jan 4, 2023

edmundito added 3 commits January 4, 2023 09:08

Clean up ConnectionPage structure

31ac6e5

Update style import in ConnectionInfoCard test

376c39e

Clean up ConnectionRoutePaths enum

d6fc808

edmundito requested a review from krishnaglick January 5, 2023 18:48

edmundito added 2 commits January 5, 2023 15:11

Merge branch 'master' of github.com:airbytehq/airbyte into edmundito/…

6fe7f39

…reorganize-connections-pages

Merge branch 'master' of github.com:airbytehq/airbyte into edmundito/…

ca080f5

…reorganize-connections-pages

edmundito requested a review from josephkmh January 5, 2023 20:56

krishnaglick approved these changes Jan 5, 2023

View reviewed changes

josephkmh reviewed Jan 10, 2023

View reviewed changes

Merge branch 'master' of github.com:airbytehq/airbyte into edmundito/…

a6a3dac

…reorganize-connections-pages

octavia-squidington-iv removed the area/platform issues related to the platform label Jan 11, 2023

edmundito requested a review from josephkmh January 11, 2023 20:31

Merge branch 'master' of github.com:airbytehq/airbyte into edmundito/…

a48d6a6

…reorganize-connections-pages

josephkmh approved these changes Jan 12, 2023

View reviewed changes

edmundito and others added 2 commits January 12, 2023 09:18

Apply suggestions from code review - Update export default style

29e01eb

Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>

Update ConnectionInfoCard test description name

de648ca

edmundito added 2 commits January 12, 2023 09:22

Merge branch 'master' of github.com:airbytehq/airbyte into edmundito/…

df72d0f

…reorganize-connections-pages

Update test snapshots

c2e84b1

edmundito enabled auto-merge (squash) January 12, 2023 14:24

edmundito merged commit 2515385 into master Jan 12, 2023

edmundito deleted the edmundito/reorganize-connections-pages branch January 12, 2023 15:00

octavia-squidington-iii mentioned this pull request Jan 12, 2023

Bump Airbyte version from 0.40.27 to 0.40.28 #21359

Merged

josephkmh mentioned this pull request Feb 16, 2023

Refactor Source Item Page to match Destination Item Page, don't load source twice #23002

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🧹 Reorganize connections pages #20845

🪟 🧹 Reorganize connections pages #20845

edmundito commented Dec 22, 2022 •

edited

Loading

josephkmh left a comment

josephkmh Jan 4, 2023

edmundito Jan 4, 2023

josephkmh Jan 4, 2023

krishnaglick Jan 5, 2023

edmundito commented Jan 5, 2023

krishnaglick left a comment

krishnaglick Jan 5, 2023

josephkmh Jan 12, 2023 •

edited

Loading

krishnaglick Jan 5, 2023

krishnaglick Jan 5, 2023

krishnaglick Jan 5, 2023

edmundito Jan 12, 2023

josephkmh left a comment

edmundito commented Jan 11, 2023

josephkmh left a comment

edmundito commented Jan 12, 2023


		describe("<StatusMainInfo />", () => {
		describe("<connectionInfo />", () => {

	describe("<connectionInfo />", () => {
	describe("<ConnectionInfoCard />", () => {

🪟 🧹 Reorganize connections pages #20845

🪟 🧹 Reorganize connections pages #20845

Conversation

edmundito commented Dec 22, 2022 • edited Loading

What

How

Recommended reading order

josephkmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Jan 5, 2023

krishnaglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

edmundito commented Jan 11, 2023

josephkmh left a comment

Choose a reason for hiding this comment

edmundito commented Jan 12, 2023

edmundito commented Dec 22, 2022 •

edited

Loading

josephkmh Jan 12, 2023 •

edited

Loading