-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
Codecov ReportPatch coverage:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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
}; | ||
|
||
const inputGroup = ( | ||
<div ref={searchInputRef}> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
setCurrentChips(newChips); | ||
}; | ||
|
||
React.useEffect(() => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
ddd78e2
to
18ed839
Compare
} | ||
|
||
export const Autocomplete: React.FC<IAutocompleteProps> = ({ | ||
// TODO: data just for testing purposes, should be removed |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
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. |
Looks really good! Great question. I think in the case of the application form, we are limiting to existing tags. |
Signed-off-by: gitdallas <dallas.nicol@gmail.com>
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). |
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.
|
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) |
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 |
![]() 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. tackle2-ui/client/src/app/pages/applications/components/application-form/application-form.tsx Lines 102 to 114 in 3c4d31e
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. |
@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. |
The enter keypress select wasn't working for me when testing this morning on Chrome - Just tested again and it is working for me! |
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.