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

refactor: searchbar rewritten as functional component #2132

Merged
merged 4 commits into from
May 15, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented May 13, 2024

Description

This PR intents to change SearchBar component into a functional component.

Changes

Rewritten SearchBar component as function leveraging useImperativeHandle to apply new methods

Test code and steps to reproduce

Checklist

@alduzy alduzy requested review from tboba and kkafar May 13, 2024 16:16
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Good job overall. I have few reflections & points to discuss, left them below.

src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
@alduzy alduzy requested a review from kkafar May 14, 2024 08:00
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

I'm nitpicking here hard, because our attention won't be back to this component for some time most likely.

Have you tested this on our example apps (there is dedicated screen in Example & FabricExample for the search bar API I believe) whether everything works fine?

src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
Copy link
Member

@tboba tboba left a comment

Choose a reason for hiding this comment

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

Good stuff! Just couple notes from me

src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
src/components/SearchBar.tsx Outdated Show resolved Hide resolved
@alduzy alduzy force-pushed the @alduzy/convert-searchbar-to-fn branch from fd0ecd5 to 33cc656 Compare May 14, 2024 13:46
@alduzy alduzy requested review from tboba and kkafar May 14, 2024 13:52
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

The code looks 🟢

Have you tested the search bar API after these changes?

@alduzy
Copy link
Member Author

alduzy commented May 15, 2024

I've tested the changes on Example app both on android and iOS and it works just like before. I didn't find any usage of the searchbar in FabricExample.

@alduzy alduzy requested a review from kkafar May 15, 2024 13:12
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

Looks like we're good to go then. Thank you 🎉

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