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

✨ Generic RepoFinder component #144

Merged
merged 6 commits into from
Jul 4, 2024
Merged

✨ Generic RepoFinder component #144

merged 6 commits into from
Jul 4, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Jul 1, 2024

Add a generic RepoFinder input field that searches by handle/did and shows profile details

@foysalit foysalit requested a review from devinivy July 3, 2024 14:22
) : (
items.map((item) => (
<Combobox.Option
key={item.handle}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works just fine, but item.did may be a more natural identifier to use.

Comment on lines +96 to +99
onChange={(item) => {
setSelectedItem(item)
onChange(item)
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a little complicated to have both internal state and external state working at the same time, as it means consumers may be prone to get out of sync with the internal state of the component. Would it be awkward to make Finder a standard controlled form component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

surely doable but only issue is that in a lot of places, we use uncontrolled inputs and then utilize web native html form methods to get data. I like that pattern for the most part and only use controlled method when absolutely needed so I left the default behavior to match that.

will do a refactor if we end up seeing issues with this.

@foysalit foysalit changed the base branch from user-typeahead to main July 4, 2024 08:55
Copy link

render bot commented Jul 4, 2024

Copy link

render bot commented Jul 4, 2024

@foysalit foysalit merged commit 54581e0 into main Jul 4, 2024
3 checks passed
@matthieusieben matthieusieben deleted the user-search-input branch November 15, 2024 14:54
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.

2 participants