Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

table sources provide table options as sorted typeahead #2939

Merged
merged 6 commits into from Feb 9, 2022

Conversation

pedroslopez
Copy link
Member

@pedroslopez pedroslopez commented Feb 3, 2022

Change description

Provides better experience for configuring the source when users have lots of tables in their database.
Also fixes typeaheads on the source edit page to play nicely alongside react-hook-form.

image

Checklists

Development

  • Application changes have been tested appropriately

Impact

  • Code follows company security practices and guidelines
  • Security impact of change has been considered
  • Performance impact of change has been considered
  • Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
  • Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

@pedroslopez pedroslopez marked this pull request as ready for review February 4, 2022 16:03
@pedroslopez pedroslopez requested review from krishnaglick and removed request for krishnaglick February 4, 2022 16:03
@pedroslopez pedroslopez marked this pull request as draft February 4, 2022 16:20
Comment on lines 470 to 471
onChange(selected[0]?.key);
updateOption(opt.key, selected[0]?.key);
Copy link
Member Author

Choose a reason for hiding this comment

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

calling both react-hook-form's onChange and our updateOption

Comment on lines 417 to 420
<Controller
control={control}
name={`source.options.${opt.key}`}
render={({ field: { onChange } }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Because Typeahead's props aren't compatible with the props react-hook-form's register function returns, we need to wrap it in a Controller with a custom render function

@@ -537,6 +551,7 @@ const Page: NextPage<Props> = ({
placeholder={opt.placeholder}
name={`source.options.${opt.key}`}
{...register(`source.options.${opt.key}`, {
shouldUnregister: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

On the first few renders until we get back the connectionOptions response, it falls back to a disabled text input. This was causing problems because register by default does not unregister on unmount, so sometimes the custom onChange function passed here stuck around and was still getting called whenever we changed the value on the typeahead. Passing shouldUnregister: true here correctly cleans up the old onChange on unmount as expected.

@pedroslopez pedroslopez marked this pull request as ready for review February 4, 2022 22:46
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.

Do you think you could write a test to test this? I feel like this is a touchy part of the codebase and could use the support.
Maybe it's worth putting this logic into its own component so it's more easily testable?

@pedroslopez
Copy link
Member Author

@krishnaglick separated out this react-hook-form + react-bootstrap-typeahead integration into its own component and added some tests for it. I was able to reuse it in both the Source Edit and App Edit pages- the destination edit page also has a typeahead in it, but the page doesn't use react-hook-form. We can probably move to using this component later when we go all-in react-hook-form.

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.

Really love what you did here, nice job!

@pedroslopez pedroslopez enabled auto-merge (squash) February 9, 2022 15:25
@pedroslopez pedroslopez merged commit 2a2e814 into main Feb 9, 2022
@pedroslopez pedroslopez deleted the 176910887-table-typeahead branch February 9, 2022 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants