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

🪟🐛 Connector builder UI: Fix inputs modal #21643

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 20, 2023

What

This PR fixes two small issues with the inputs modal:

  • The default value input type is not updated correctly when the type changes
  • Placeholder is not reflected in the test input form

How

For the first one, I tried for a bit to find a "real" solution, but it became way to complex to justify it here. The problem is that formik will not re-render the inner field if the value doesn't change which means changes in the props after the initial render might not be picked up correctly. I tried to come up with a generic way to enforce an update for any of the props, but there are two issues with this:

  • There are a lot of props which are not memoized, so we would need to take care of that first which makes the change pretty big (otherwise the full thing would just re-render all the time as before)
  • It's pretty difficult to force formik to re-render in this situation and the complexity doesn't seem justified - we basically need to re-implement the regular formik fastfield "shouldUpdate" logic, then do a standard memoize check on the passed down props as well. To do this in a performant way isn't super simple, there are libraries for that but it doesn't look like it's worth it.

The solution I went with is adding a key prop to the fastfield with the type value so it will definitely re-render in this situation. It fixes the problem at hand and I hope we can soon refactor this code - with react-hook-form we won't have this problem.

For the second one - I checked and you are right, we don't support placeholders like this. I think this was a left-over from working from the original designs that included that field. I removed it.

Joe Reuter added 2 commits January 20, 2023 12:50
@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 20, 2023
@flash1293 flash1293 marked this pull request as ready for review January 20, 2023 12:42
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.

Changes seem fine to me. I agree this doesn't feel like the "correct" way to solve this, but it is very minimal and should be okay for now until we switch to a different form library.

Tested locally and it seems to fix the issue

@lmossman
Copy link
Contributor

Merging now since I am going to release OSS and I want this fix included

@lmossman
Copy link
Contributor

/approve-and-merge reason="CI passed on previous commit, I just clicked Update branch by accident. Merging now since I am about to trigger an OSS release and want this change included"

@octavia-approvington
Copy link
Contributor

Lets do it!
giddddy-up

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 team/extensibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants