gif-picker-react icon indicating copy to clipboard operation
gif-picker-react copied to clipboard

Adding controlled picker Context

Open manoelneto opened this issue 1 year ago • 3 comments

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

manoelneto avatar Aug 06 '24 15:08 manoelneto

@MrBartusek can you check that?

manoelneto avatar Aug 08 '24 19:08 manoelneto

@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?

MrBartusek avatar Aug 09 '24 08:08 MrBartusek

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

manoelneto avatar Aug 09 '24 10:08 manoelneto

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?

MrBartusek avatar Aug 22 '24 07:08 MrBartusek

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.

manoelneto avatar Aug 22 '24 10:08 manoelneto

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.

MrBartusek avatar Aug 22 '24 10:08 MrBartusek

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 avatar Aug 22 '24 11:08 manoelneto

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.

manoelneto avatar Aug 22 '24 11:08 manoelneto

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

MrBartusek avatar Aug 22 '24 11:08 MrBartusek

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

TheNoobiCat avatar Sep 05 '24 17:09 TheNoobiCat

@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.

jaxomlotus avatar Sep 09 '24 17:09 jaxomlotus

@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.

maiconcarraro avatar Sep 17 '24 20:09 maiconcarraro

@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.

jaxomlotus avatar Sep 17 '24 20:09 jaxomlotus

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.

TheNoobiCat avatar Sep 17 '24 20:09 TheNoobiCat

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.

maiconcarraro avatar Sep 17 '24 20:09 maiconcarraro

@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.

MrBartusek avatar Sep 17 '24 21:09 MrBartusek

@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

maiconcarraro avatar Sep 17 '24 22:09 maiconcarraro

@manoelneto you have skipped some of my comments

MrBartusek avatar Sep 18 '24 12:09 MrBartusek

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

MrBartusek avatar Sep 18 '24 12:09 MrBartusek

@manoelneto you have skipped some of my comments

done. sorry about that.

manoelneto avatar Sep 18 '24 12:09 manoelneto

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

it's already done

manoelneto avatar Sep 18 '24 12:09 manoelneto

@manoelneto and @MrBartusek THANK YOU!!!

jaxomlotus avatar Sep 18 '24 15:09 jaxomlotus

Yay 🎉

TheNoobiCat avatar Sep 18 '24 15:09 TheNoobiCat

Thank you!

MrBartusek avatar Sep 21 '24 09:09 MrBartusek

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

MrBartusek avatar Sep 21 '24 10:09 MrBartusek

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: @.***>

jaxomlotus avatar Sep 22 '24 02:09 jaxomlotus