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

feat(HMS-2301): add query search for instance types select #299

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

amirfefer
Copy link
Member

This PR adds a query search filtering to the instance types select input, using the jsonata query language for parsing and execution.

Demo:

Screen.Recording.2023-08-08.at.20.27.57.mov

@lzap
Copy link
Member

lzap commented Aug 9, 2023

Perfect. Love it.

@@ -132,7 +144,8 @@ const InstanceTypesSelect = ({ setValidation, architecture }) => {
selections={chosenInstanceType}
onToggle={onToggle}
onSelect={onSelect}
onFilter={onFilter}
onFilter={() => {}}
Copy link
Member Author

Choose a reason for hiding this comment

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

According to pf docs, onFilter returns the actual filtered options, and it is triggered multiple times, making it a bad practice for state changes.

onTypeaheadInputChanged is triggered only after a change in the typeahead input, with throttling it optimized and works smoothly, due to the implementation of the PF select, we still need an empty onFilter callback

Copy link
Member

Choose a reason for hiding this comment

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

I'm missing the point why have we choose the onTypeaheadInputChanged instead of onFilter? 🤔
Could you provide more details (maybe even in the PR description for better visibility)?

Copy link
Member Author

@amirfefer amirfefer Aug 17, 2023

Choose a reason for hiding this comment

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

Sure,

We used onFilter incorrectly, we change the state instead of returning a filtered array.
By design, onFilter is not a callback for asynchronous change, like updating the state, therefore we can't use onFilter with jsonata because it is an async processing.

on the other hand, onTypeaheadInputChanged is a callback that triggers only when the typeahead input has changed, by design it supports asynchronous calls, and it works well for changing the state and executing async queries.

Copy link
Member

@avitova avitova left a comment

Choose a reason for hiding this comment

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

The query gets deleted on click after inserting it. I would not expect such behaviour, tbh. If there was a typo, the only way to edit the query would be using a keyboard.
Should we have a state for the query?

@amirfefer
Copy link
Member Author

Thanks, @avitova for the review, you're right.
We can't control the typeahead's input value, but we can make it persistent by using isInputValuePersisted prop, should work now as you expected :)

@avitova
Copy link
Member

avitova commented Aug 21, 2023

@amirfefer Thank you for looking into my review. I can see that the filtering results are now visible even after the filtering input disappears. The situation I was worried about more was, however, when a user types in an invalid filtering value and tries to edit a typo.
In my case, I typed something like vcpu=2, I found out it was supposed to be vcpus=2 instead, clicked into the correct position of the input field to correct myself, and the query has been deleted entirely.
Also, I might want to try editing the query with the cursor while the query is not finished, and the query would get deleted, in which case, the isInputValuePersisted would not help that much, as it would show the results of an unfinished query.
If you are saying we can not control this, then this solution helps at least with some of the problems. 😆

@amirfefer
Copy link
Member Author

Thanks @avitova, fixed. using the cursor doesn't delete the search query.
Still, we can't control the typeahead's search value in the select component, but there is a way to bypass it :)

Rebased.

@ezr-ondrej
Copy link
Member

Works pretty well, the issue is still there after we select a value, but I'd say it's not as bad now.

It's pretty usable until we select the value and I'd rather have this then have the search override the selected value, so I guess this is great start :)

Full test - first part is excelent, the last part is bit weird, but I guess makes sense and would be hard to fix:

Screencast.from.2023-08-29.15-33-49.webm

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Thanks @amirfefer and @avitova ! 🎉

@ezr-ondrej ezr-ondrej merged commit af81237 into RHEnVision:main Aug 30, 2023
3 checks passed
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.

4 participants