MrBartusek / gif-picker-react

Tenor GIF Picker component for React ⚛️
http://dokurno.dev/gif-picker-react/
MIT License
38 stars 12 forks source link

Add `initialSearchTerm` prop #32

Closed manoelneto closed 1 month ago

manoelneto commented 3 months ago

This allows to control the search term by overriding the pickerContextValue.

manoelneto commented 2 months ago

@MrBartusek can you check that?

MrBartusek commented 2 months ago

@manoelneto Hey, thanks for the PR. What is the motivation for this change, and how would it help our users? What are the use cases for this property?

manoelneto commented 2 months ago

the idea is to make the search term controlled, so we can pass initial term for example.

MrBartusek commented 2 months ago

the idea is to make the search term controlled, so we can pass initial term for example.

What are the use cases of this? Can you give examples of how would it benefit the developer using this picker?

manoelneto commented 2 months ago

Hey @MrBartusek, I'm not entirely sure if my solution is perfect, but I think it makes sense for the parent component to control the query term if needed. So, I came up with this approach that doesn't require too much code change.

By the way, I noticed that in the Storybook, the way you're passing the initial query is a bit off—it seems like you're simulating DOM events to change the term, which feels a bit against React patterns.

MrBartusek commented 2 months ago

I'm not entirely sure if my solution is perfect, but I think it makes sense for the parent component to control the query term if needed. So, I came up with this approach that doesn't require too much code change.

I want to know how would it be usefull to a developer. In which situation would anyone want to control the internal search term here

By the way, I noticed that in the Storybook, the way you're passing the initial query is a bit off—it seems like you're simulating DOM events to change the term, which feels a bit against React patterns.

These are internal test files, they are ment to simulate user actions.

manoelneto commented 2 months ago

It's very usefull. When we render the gif component, we pass the initial query term. As in this example, sending a shoutout in our platform.

Screenshot 2024-08-22 at 08 01 11

We also have other use cases (celebrating birthdays) that is also usefull.

manoelneto commented 2 months ago

Unrelated, not sure if you noticed, but I also added the powered by tenor image below the searchbar to our internal fork, which is also necessary to complain with tenor terms.

MrBartusek commented 2 months ago

Unrelated, not sure if you noticed, but I also added the powered by tenor image below the searchbar to our internal fork, which is also necessary to complain with tenor terms.

For that reffer to: https://github.com/MrBartusek/gif-picker-react#what-to-know-before-using

TheNoobiCat commented 2 months ago

I think this would be a great addition to the library and it looks quite useful to be honest

jaxomlotus commented 1 month ago

@MrBartusek I agree that this is a useful addition. Currently the only reason I have not implemented this library is that I have no way to provide this context when the library opens. So I get a list of unrelated gifs by default, instead of a list of gifs related to the chat message the user had already started typing. Please allow this functionality to exist or for a developer to at least have a way to override the default text so we can accomplish it. Thank you.

maiconcarraro commented 1 month ago

@MrBartusek I can share an use case from our product, we have creators that upload to Tenor their own gallery of gifs, and all of their gifs have a common tag that is related to our product's name, so the initial search for the users is using this same common tag so they can easily pick without having to search... but they can still search if they want, just a matter of UX.

We're migrating from Giphy to Tenor, let me know if there is a better approach if you don't agree to the initial search query.

jaxomlotus commented 1 month ago

@MrBartusek I had a similar use case above. And as Giphy has now introduce a pricing plan, I suspect lots of people will be migrating to Tenor, with similar requests.

For what it's worth, if the sole reason for not implementing this is concern over where you can post attribution for Tenor, just append a powered by Tenor statement right under the search input, as in @manoelneto screenshot above, or rely on the user to figure out where to post attribution and stay compliant. It will still look acceptable.

TheNoobiCat commented 1 month ago

Yeah, maybe a small label with "Tenor Results" under the search bar could work. Or, as @jaxomlotus said, just require the users to manage attribution.

maiconcarraro commented 1 month ago

I didn't find in their terms, so suggesting here (lmk if it's agains't), but you can still show "Search Tenor" and allow initial search query to render/filter the relevant initial gifs, but once the user start typing that's what takes precedence.

Initial search query is not the same as initial input value IMHO, for our product we only need to filter initial results and not control the input value itself.

MrBartusek commented 1 month ago

@jaxomlotus @TheNoobiCat For the attribution concerns - according to the atribution guide I wouldn't worry too much about it

Search Tenor. Use this attribution as the placeholder text in your search box.

We do have a placeholder and the fact that we put some default text in the search box on render doesn't change that.

maiconcarraro commented 1 month ago

@MrBartusek I sent a different PR based on my original suggestion in case you agree/prefer the approach https://github.com/MrBartusek/gif-picker-react/pull/33

MrBartusek commented 1 month ago

@manoelneto you have skipped some of my comments

MrBartusek commented 1 month ago

@manoelneto please also change Search story to use this new prop

manoelneto commented 1 month ago

@manoelneto you have skipped some of my comments

done. sorry about that.

manoelneto commented 1 month ago

@manoelneto please also change Search story to use this new prop

it's already done

jaxomlotus commented 1 month ago

@manoelneto and @MrBartusek THANK YOU!!!

TheNoobiCat commented 1 month ago

Yay 🎉

MrBartusek commented 1 month ago

Thank you!

MrBartusek commented 1 month ago

I've just published v1.4.0 please let me know if it works!

jaxomlotus commented 1 month ago

It works great! Thank you.

On Sat, Sep 21, 2024 at 6:39 AM Bartosz Dokurno @.***> wrote:

I've just published v1.4.0 please let me know if it works!

— Reply to this email directly, view it on GitHub https://github.com/MrBartusek/gif-picker-react/pull/32#issuecomment-2365138969, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADWSSWJJLKNMIJ7CIGN3EDZXVEGNAVCNFSM6AAAAABMCRHCYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRVGEZTQOJWHE . You are receiving this because you were mentioned.Message ID: @.***>