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.

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

🪟🔧 Refactor ConnectorsView #21531

Merged
merged 14 commits into from
Jan 26, 2023
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 18, 2023

What

Fixes #20706

How

The following changes have been done:

  • Remove styled component from and extend the indicator component so it can render invisible - this makes it easy for things to be aligned which have a leading optional indicator like in the connectors table
  • Instead of rendering a custom string for custom connectors, use a badge like for alpha and beta connectors
  • Show a loading indicator in the "Upgrade" button while a single connector is updating
  • Do not show the loading indicator for connectors that don't actually update when hitting "Upgrade all" (only the ones that are actually upgradable at the moment)
  • Remove styled components from different parts of the connectors table
  • To prevent often occurring re-render of full table, introduce a context to pass down the loading state to each individual loading button
  • Use standard components for the "Add custom connector" dialog
  • Always show the "Upgrade all" button so the layout doesn't jump due to it

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 18, 2023
@flash1293 flash1293 changed the title WIP Refactor connectorview Refactor ConnectorsView Jan 19, 2023
@flash1293 flash1293 changed the title Refactor ConnectorsView 🪟🔧 Refactor ConnectorsView Jan 19, 2023
@flash1293 flash1293 marked this pull request as ready for review January 19, 2023 16:54
@flash1293 flash1293 requested a review from lmossman January 23, 2023 19:46
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

A couple questions but nothing truly blocking. LGTM and seems to work well from local testing

Comment on lines +18 to +19
const feedbackListRef = useRef(feedbackList);
feedbackListRef.current = feedbackList;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change made? And is it intentional that this change was only made to SourcesPage and not also to DestinationsPage?

Side note: it seems like the logic in those two components is almost exactly the same, so a lot of it could be moved into the ConnectorsView component, with props to differentiate between the source- or destination-specific methods. What do you think? Could also be done in a separate PR if you'd prefer - just thought I'd call it out here since we are already refactoring in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant to do the same change for the destination, thanks for catching this. This is done to prevent a full re-render of the table on state change of the feedback list. If onUpdateVersion changes, then renderColumns changes here

[allowUpdateConnectors, allowUploadCustomImage, onUpdateVersion]

which in turn means the columns object passed to the table changes here:

which should be avoided (the docs say that the columns should be memoized: https://react-table-v7.tanstack.com/docs/api/useTable which means if they change reference it's pretty expensive)

</Formik>
</Content>
</Modal>
<Formik
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why did you move the Formik outside of the Modal?

@flash1293 flash1293 merged commit 7f36386 into master Jan 26, 2023
@flash1293 flash1293 deleted the flash1293/refactor-connectorview branch January 26, 2023 15:48
pgollangi pushed a commit to pgollangi/airbyte that referenced this pull request Jan 26, 2023
* WIP

* WIP

* wip

* more fixes

* fix test

* remove some more styled components and improve memoization

* review comment
archangelic added a commit that referenced this pull request Apr 27, 2023
* feat: source-gitlab accept api_url w/ or w/o scheme

* 🪟🔧 Refactor ConnectorsView (#21531)

* WIP

* WIP

* wip

* more fixes

* fix test

* remove some more styled components and improve memoization

* review comment

* feat: source-gitlab accept api_url w/ or w/o scheme

* Revert allowedHosts missing value checks (#21923)

* bump versions

* update expected records

* auto-bump connector version

---------

Co-authored-by: Joe Reuter <joe@airbyte.io>
Co-authored-by: Evan Tahler <evan@airbyte.io>
Co-authored-by: Mal Hancock <mallory@archangelic.space>
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
marcosmarxm added a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
* feat: source-gitlab accept api_url w/ or w/o scheme

* 🪟🔧 Refactor ConnectorsView (airbytehq#21531)

* WIP

* WIP

* wip

* more fixes

* fix test

* remove some more styled components and improve memoization

* review comment

* feat: source-gitlab accept api_url w/ or w/o scheme

* Revert allowedHosts missing value checks (airbytehq#21923)

* bump versions

* update expected records

* auto-bump connector version

---------

Co-authored-by: Joe Reuter <joe@airbyte.io>
Co-authored-by: Evan Tahler <evan@airbyte.io>
Co-authored-by: Mal Hancock <mallory@archangelic.space>
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: marcosmarxm <marcosmarxm@gmail.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor ConnectorsView
3 participants