Add FilterSearch component #56
Conversation
| const requestId = useRef(0); | ||
| const responseId = useRef(0); | ||
|
|
||
| async function executeFilterSearch (inputValue: string) { |
There was a problem hiding this comment.
I think we can use the useAutocomplete hook but maybe call it useSynchronizedSearch (there's probably a better name) and takes in a function to execute either autocomplete or filtersearch.
There was a problem hiding this comment.
abstracting out this logic so that we don't duplicate it makes sense to me
| const filterSearchFields = [{ | ||
| fieldApiName: 'builtin.location', | ||
| entityType: 'ce_person', | ||
| fetchEntities: false |
There was a problem hiding this comment.
If we enable fetchEntities, it won't affect the FilterSearch component at all, correct? Because of that, I'm thinking we shouldn't have fetchEntities as an option for the component. We could use a Typescript utility type to omit the field, and inside the component we can always set it to false
| focusedOptionIndex, | ||
| optionIdPrefix, | ||
| cssClasses | ||
| }: Props): JSX.Element | null { | ||
| function renderOption(option: Option, index: number) { | ||
| function renderSection(section: {results: Option[], label?: string}, sectionIndex: number) { | ||
| return ( |
There was a problem hiding this comment.
Do we want to enforce that each section must consist of an array of results? We could make it more generic by allowing an array of sections to be passed in which define their own render function similarly to how options can be passed in. I believe Tom mentioned a use case such as Visual Autocomplete where we'd want sections, but they wouldn't necessarily be laid out this way. Have you checked with Tom on the use cases here?
| @@ -8,14 +8,17 @@ interface Props { | |||
| placeholder?: string, | |||
| screenReaderInstructions: string, | |||
| screenReaderInstructionsId: string, | |||
| options: Option[], | |||
| options: {results: Option[], label?: string}[], | |||
There was a problem hiding this comment.
This changes forcers consumers of this component to organize the dropdown into sections. Is there a way we could avoid that?
There was a problem hiding this comment.
The only way I could think of to avoid that would be to keep the options array as is, but pass in an optional array of strings and numbers for the labels of each section and the number of results in each section. This would assume that the options array is ordered by section in the same order as the labels and counts.
Something like additionalOptionsData?: {labels: string[], counts: number[]} or
additionalOptionsData?: {label: string, count: number}[]
But that seemed a little more confusing/difficult to maintain
There was a problem hiding this comment.
another way would be allowing for both sections and the previous array of options
options: {results: Option[], label?: string}[] | Option[]
and either InputDropdown support both logic, or transform options to a section internally if it's not one
There was a problem hiding this comment.
How about keeping the options data the same but making it optional, and adding an optional sections param with a different interface?
There was a problem hiding this comment.
would it seems a little odd if there's two params for passing options?
…answers-headless-react into dev/filtersearch-component
| @@ -97,24 +121,43 @@ export default function InputDropdown({ | |||
| evt.preventDefault(); | |||
| } | |||
|
|
|||
| const isFirstSectionFocused = focusedSectionIndex === 0; | |||
There was a problem hiding this comment.
Adding sections support is adding a lot of complexity to this logic. If we support either an array of options or a sections object, then we could have separate functions where one handles the logic for simple options, and the other handles the logic for sections.
| focusedOptionIndex, | ||
| optionIdPrefix, | ||
| cssClasses | ||
| }: Props): JSX.Element | null { | ||
| function renderOption(option: Option, index: number) { | ||
| function renderSection(section: { results: Option[], label?: string }, sectionIndex: number) { |
There was a problem hiding this comment.
I might consider moving these functions to the top level of the file. I don't think it matters in this case, but react uses reference equality in a number of places, and here new renderSection() and renderOption() functions are used for each render of Dropdown. If we move them to the top level the functions will have stable references. AGAIN I don't think matters here because we are not passing these functions in as props anywhere or using them in as memo dependencies etc. but I also don't have a compiler in my brain
| function handleDocumentClick(evt: MouseEvent) { | ||
| const target = evt.target as HTMLElement; | ||
| if (!target.isSameNode(inputRef.current)) { | ||
| if (!(target.isSameNode(inputRef.current) || target.classList.contains(cssClasses.optionContainer) |
There was a problem hiding this comment.
The cssClasses object may contain a list of classes, so classList.contains wouldn't work in that scenario. Can we check in some other way that doesn't rely on the classList?
There was a problem hiding this comment.
The other way I thought of would involve useRef and forwardRef, but I think there would need to be a reference for each section label
Create a component for FilterSearch. Update InputDropdown and Dropdown to have sections of dropdown options.
J=SLAP-1643
TEST=manual
Conduct filter searches in sample-app to check the behavior is as expected. Check that search bar dropdown functions as before.