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

✨ add autocomplete component #1295

Merged
merged 15 commits into from
Aug 31, 2023
Merged

Conversation

gitdallas
Copy link
Collaborator

@gitdallas gitdallas commented Aug 17, 2023

closes #1266

it is looking mostly like the images in the story (made assumptions on the dropdown menu).

may need some tweaks depending on needs, but wanted to get a PR up before i head out for the long weekend.

image

image

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Patch coverage: 33.55% and project coverage change: -0.15% ⚠️

Comparison is base (9063539) 43.07% compared to head (29054d2) 42.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1295      +/-   ##
==========================================
- Coverage   43.07%   42.92%   -0.15%     
==========================================
  Files         145      146       +1     
  Lines        4325     4454     +129     
  Branches      998     1040      +42     
==========================================
+ Hits         1863     1912      +49     
- Misses       2450     2530      +80     
  Partials       12       12              
Flag Coverage Δ
client 42.92% <33.55%> (-0.15%) ⬇️
server ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
client/src/app/components/Autocomplete.tsx 33.09% <33.09%> (ø)
...s/components/application-form/application-form.tsx 59.60% <40.00%> (+4.20%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@avivtur avivtur left a comment

Choose a reason for hiding this comment

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

Hi there :)
Great work on this!
I've added some comments

client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
};

const inputGroup = (
<div ref={searchInputRef}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can assign the ref directly to the SearchInput component and drop the div as it's not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for whatever reason, if i do this here it will not open the menu

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why that happens, I'll try to checkout this PR locally and see if I can figure out what's happening to the ref here

client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
setCurrentChips(newChips);
};

React.useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain why is this useEffect hook needed?
useEffect is meant to be used with side effects, this looks like the things running here can be inserted into the handleInputChange method, as it changes on every change of inputValue as it seem on the dependency array at line 116

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

side effect of both a change in current chips or a change in the search now

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to update a component’s state when some props or state change, You shouldn’t need an Effect. Removing unnecessary Effects will make your code easier to follow, faster to run, and less error-prone.
https://react.dev/learn/you-might-not-need-an-effect

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gitdallas Please refer to this comment in your follow-up PR, Thanks!

client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
client/src/app/components/Autocomplete.tsx Outdated Show resolved Hide resolved
@gitdallas gitdallas force-pushed the feature/autocomplete-form-field branch from ddd78e2 to 18ed839 Compare August 24, 2023 18:31
@gitdallas gitdallas requested a review from avivtur August 25, 2023 12:49
}

export const Autocomplete: React.FC<IAutocompleteProps> = ({
// TODO: data just for testing purposes, should be removed
Copy link
Member

@ibolton336 ibolton336 Aug 25, 2023

Choose a reason for hiding this comment

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

Could you implement this in a field for testing purposes as a part of this PR? Maybe on one of the searchable dropdowns in the applications form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ibolton336 - it's being used for tags field now

@gitdallas
Copy link
Collaborator Author

While testing this out on the tags field.. I'm wondering if this is meant to allow "new" options or if the user should be restricted to the available options.

@ibolton336
Copy link
Member

Looks really good! Great question. I think in the case of the application form, we are limiting to existing tags.

@gitdallas
Copy link
Collaborator Author

I went ahead and made it configurable with a flag prop. Also made it so that if a user doesn't pass in a menu header, it will not have one (more similar to how the business service dropdown looks).

@ibolton336
Copy link
Member

Looking great. Not required but would be cool to be able to select with enter key press.

Also, I think we will need to be able to view by category somehow in the dropdown here.

image
https://github.com/konveyor/enhancements/blob/867bd8590c18ea877499350a258b1fcbea2e2ef5/enhancements/assessment-module/README.md

@gitdallas
Copy link
Collaborator Author

gitdallas commented Aug 29, 2023

Enter by keypress will work. If you are not allowing user to input custom labels, it will add a new label with whatever the user has. If you are not allowing custom labels, it will only work if the text matches (case insensitive) an option or if there is a hint given.

You can also select with "enter" keypress on the menu itself (open with right arrow key, down arrow to select something, enter to finish)

@gitdallas
Copy link
Collaborator Author

gitdallas commented Aug 29, 2023

As for the menu changes, I wish I would have known about this when I was starting the story. It looks like it will require quite a bit of change... the checkboxes and all. I'm also unsure how the categories work - what will the data look like? @ibolton336

@ibolton336
Copy link
Member

ibolton336 commented Aug 29, 2023

As for the menu changes, I wish I would have known about this when I was starting the story. It looks like it will require quite a bit of change... the checkboxes and all. I'm also unsure how the categories work - what will the data look like? @ibolton336

Screenshot 2023-08-29 at 1 33 58 PM

This is example of a tagCategory with tags as it exists today in the controls panel of the app. Here is the data model

Looks like currently the tags in the application form are pulled from the tag categories and flattened to create the tag options.

useEffect(() => {
if (tagCategories) {
setTags(tagCategories.flatMap((f) => f.tags || []));
}
}, []);
const tagOptions =
tags?.map((tag) => {
return {
value: tag.name,
toString: () => tag.name,
};
}) || [];

If you could work in the category data structure that would be a great improvement. But it isn't clearly defined in the initial issue -appolgies for that. We can push that to a seperate issue if you prefer. The enhancement doc might provide some more clarity but I view the checkboxes as a nice-to-have feature as long as multi-select is working.

@gitdallas
Copy link
Collaborator Author

gitdallas commented Aug 29, 2023

@ibolton336 - if you don't mind, i would prefer that being moved to another story. I'd like to focus more on removing deprecated dependencies.

@ibolton336
Copy link
Member

@ibolton336 - if you don't mind, i would prefer that being moved to another story. I'd like to focus more on removing deprecated dependencies.

Alright fair enough! Opened #1320 up that can be worked on now. The deprecated stuff can wait for out bug fix cycle after feature freeze. We should prioritize custom assessment feature work until then.

@ibolton336
Copy link
Member

ibolton336 commented Aug 29, 2023

Enter by keypress will work. If you are not allowing user to input custom labels, it will add a new label with whatever the user has. If you are not allowing custom labels, it will only work if the text matches (case insensitive) an option or if there is a hint given.

You can also select with "enter" keypress on the menu itself (open with right arrow key, down arrow to select something, enter to finish)

The enter keypress select wasn't working for me when testing this morning on Chrome - Just tested again and it is working for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Autocomplete form field
3 participants