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

fix(1055): Added TypeaheadEditor and reusing it with DataFormatEditor #1118

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

shivamG640
Copy link
Contributor

Fixes #1055,

Creating this PR for initial review on the TypeaheadEditor reusable component, currently used with DataFormatEditor. Once reviewed, we can reuse the new componet with loadbalancer and ExpressionEditor as well.

This set of new changes can be tested here: https://shivamg640.github.io/kaoto/

@shivamG640
Copy link
Contributor Author

@lordrip , Please have a look at it!

Copy link
Member

@lordrip lordrip left a comment

Choose a reason for hiding this comment

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

The extraction part seems good. I tried the live instance and was also good, we need to change a bit the approach in terms of what do we pass in the TypeaheadEditor.

I think in this case, TypeaheadEditorshouldn't have any knowledge about the input, at most, it should receive a list of name and description, then the consumer of this new component, should match the selected name with the input list.

As an example, for the DataformatEditor, it should pass:

{
  name: string
  description: string
}[]

Then, it should also pass a onSelect callback, so when the user picks something from the dropdown, the DataformatEditor will match it with the available list

@shivamG640
Copy link
Contributor Author

Hi @lordrip , Thanks for the review!
I think we are already doing what you stated above. The new TypeaheadEditor is getting the select options, the current selection, current selection model and schema as input. Also we are handling the selection change in the DataFormatEditor component and assigning the new dataformat with it model and schema there.
https://github.com/shivamG640/kaoto/blob/4b79c751b32aebf5bd7095b30ce10cb85dadf87b/packages/ui/src/components/Form/dataFormat/DataFormatEditor.tsx#L60-L67

@shivamG640
Copy link
Contributor Author

Added a commit which reuse the TypeaheadEditor with LoadBalancerEditor. Changes are deployed here: https://shivamg640.github.io/kaoto/

@lordrip
Copy link
Member

lordrip commented Jun 2, 2024

Hi @lordrip , Thanks for the review! I think we are already doing what you stated above. The new TypeaheadEditor is getting the select options, the current selection, current selection model and schema as input. Also we are handling the selection change in the DataFormatEditor component and assigning the new dataformat with it model and schema there. https://github.com/shivamG640/kaoto/blob/4b79c751b32aebf5bd7095b30ce10cb85dadf87b/packages/ui/src/components/Form/dataFormat/DataFormatEditor.tsx#L60-L67

Yes, but notice how we're passing the underlying typings to TypeaheadEditor component here: https://github.com/KaotoIO/kaoto/pull/1118/files#diff-4c8da342242caefe1208f80e23b1579df1e9d6327421ee6337307cf84aeebfeaR33

The goal is to be able to reuse this component as much as possible, but in our current proposal, we need to add additional typings every time we need to use the TypeaheadEditor component somewhere else.

My proposal would be to have a standard model, like the one I mentioned before, and let the consumers of this component to adjust their values to the standard one.

Please let me know if you want to go a bit deeper on this topic.

@shivamG640
Copy link
Contributor Author

shivamG640 commented Jun 3, 2024

@lordrip,
I might need your help to correct the styling a bit in case of Expression Editor:
image

@lordrip
Copy link
Member

lordrip commented Jun 4, 2024

@lordrip, I might need your help to correct the styling a bit in case of Expression Editor

This is because of the <Card/> component that we're using in the <TypeaheadEditor/> component. My suggestion would be:

  1. Remove the <Card/> from the <TypeaheadEditor/>
  2. Remove the title from props
  3. Add the <Card/> and the title to the DataformatEditor and LoadBalancerEditor with the corresponding data-testid attribute

After doing this, we need to add a class to <SelectList id="select-typeahead-listbox"> so it can limit the height of the menu, perhaps something like this:

.typeahead-list {
  max-height: 400px;
  overflow-y: scroll;
}

This should be enough.

@shivamG640 shivamG640 marked this pull request as ready for review June 4, 2024 14:50
@lordrip lordrip merged commit ba55170 into KaotoIO:main Jun 4, 2024
9 checks passed
@shivamG640 shivamG640 deleted the Fix_1055 branch June 4, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a Typeahead component and use it in DataFormatEditor, ExpressionEditor, and LoadBalancerEditor
2 participants