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

feat: allow to set initialSearchTerm #33

Closed
wants to merge 2 commits into from

Conversation

maiconcarraro
Copy link

Based on discussion on this other PR #32 I decided to send a suggestion to have a better UX + fixing some typos.

Behavior:

  • Placeholder will still show Search Tenor.
  • Initial search term will be the default and initial search to already show relevant/context gifs to the user.
  • Categories won't show up because of the initialSearchTerm, added this note to the README docs.

image

const tenor = useContext(TenorContext);

useEffect(() => {
setLoading(true);
async function search(): Promise<any> {
const result = await tenor.search(searchTerm);
const result = await tenor.search(searchTerm || pickerContext.initialSearchTerm);
Copy link
Author

Choose a reason for hiding this comment

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

Initially I thought about adding the logic to the TenorManager but since this is the actual place that triggers the search it was looking better to leave this logic here.

Comment on lines +4 to +6
searchTerm: string;
showTrending: boolean;
initialSearchTerm: string;
Copy link
Author

Choose a reason for hiding this comment

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

I'm using VS code default format, it was odd to the previous code, seems okay now

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, i will work on this kind of issues #34

Comment on lines +9 to +11
export type PickerContextSettings = {
initialSearchTerm: string;
}
Copy link
Author

Choose a reason for hiding this comment

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

following the pattern from useSettings

Copy link
Owner

@MrBartusek MrBartusek left a comment

Choose a reason for hiding this comment

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

initialSearchTerm implementation is just a duplicate of #32 at this point. But I would gladly merge these typo fixes. Please remove initialSearchTerm implementation from here but keep other fixes.

@maiconcarraro
Copy link
Author

maiconcarraro commented Sep 18, 2024

@MrBartusek not the same, it still has a different behavior related to have input value empty + still search relevant gifs, you can see the storybook, would you rather the other behavior?

@MrBartusek
Copy link
Owner

initialSearchTerm is just a duplicate of #32 at this point. But I would gladly merge these typo fixes. Please remove initialSearchTerm implementation from here but keep other fixes.

it still has a different behavior related to have input value empty + still search relevant gifs, you can see the storybook, would you rather the other behavior?

Oh, I overlooked that. In that case, it's just a questionable UX choice. A user can see that nothing is being searched for and then they see a search result. I would prefer #32 implementation rather than this one

Can you please tell me, does #32 fit your use case?

@maiconcarraro
Copy link
Author

@MrBartusek Technically so so, it has a worst UX, our common tag used w/ creators is the name of the product, which would make no sense (or irrelevant) to show to the user as a term, they will need to remove the entire term to search something else, whereas this one will friendly change behavior based on users' interaction

@MrBartusek
Copy link
Owner

MrBartusek commented Sep 21, 2024

Hey! I've just merged #32 and the implementation for the initialSearchTerm as in this PR doesn't work for this project. I would be happy to merge a PR with these typo changes you have made, and we can discuss a fix for your use case later.

This may be tricky to update this PR specifically since I've implemented new lint rules and caused conflicts in every single file. So I would suggest you either make a new PR with the typo fixes or try to change this one.

As for your use case, please add me on discord @mrbartusek I would like to see your product and try to work out a solution.

@maiconcarraro
Copy link
Author

@MrBartusek thank you, I'll take a look, lets close this one.

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