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

Projects list search #72

Merged
merged 3 commits into from
Feb 22, 2024
Merged

Projects list search #72

merged 3 commits into from
Feb 22, 2024

Conversation

scrummish
Copy link
Contributor

Hey yall, this is the search bar component.
Let me know your thoughts!

@scrummish scrummish requested review from mbwatson and Woozl November 28, 2023 14:15
Copy link
Member

@Woozl Woozl left a comment

Choose a reason for hiding this comment

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

Looks pretty good. There seems to be a slight bug if I reload the page and then search an item and press enter on it in the list, which then displays the whole unfiltered list:

Screen.Recording.2023-11-28.at.10.24.55.AM.mov

I wonder though if the autocomplete is needed here. I think a simple text input that filters and sorts the list would probably suffice, especially if the matching text could be highlighted or bolded in the project cards.

<SearchBar
searchQuery={searchQuery}
setSearchQuery={setSearchQuery}
options={projectsFiltered}
Copy link
Member

Choose a reason for hiding this comment

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

I think the autocomplete has the search and filtering functionality built-in to display the list of matching items, so I don't think it makes sense to filter the options? I may be wrong though.

@mbwatson
Copy link
Member

mbwatson commented Nov 28, 2023

good bug-find, @Woozl. i missed that.

i'm wondering, too, now about autocomplete here. it seems that the filtered cards and filtered terms are the same list, so a simple filter/search would likely suffice.

Copy link
Member

@mbwatson mbwatson left a comment

Choose a reason for hiding this comment

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

after some thought, @scrummish, let's drop the autocomplete (as it's redundant functionality in this instance), and let's just have a plain ol' text input filter situation. can you make this adjustment?

Copy link
Member

@mbwatson mbwatson left a comment

Choose a reason for hiding this comment

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

looks good. i think it's cleaner, but i think there's more to clean out with autocomplete gone. i don't see a difference regardless of whether StyledAutocompleteSearch is used. additionally, that options prop is now obsolete.

lastly, there's a fullWidth is prop available on FormControl, so the width: '100%' stuff can go away if that's used.

these changes should simplify the component and clean out the leftover autocomplete artifacts.

Copy link
Member

@Woozl Woozl left a comment

Choose a reason for hiding this comment

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

👍

@scrummish scrummish merged commit a26ebe2 into develop Feb 22, 2024
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.

3 participants