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

Focus search input on Search tab soft reset #5732

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Conversation

rshigg
Copy link
Contributor

@rshigg rshigg commented Oct 11, 2024

Demo

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-11.at.20.50.28.mp4

Why?

This is the way the search tab works in many apps. Most notably this is something I miss from the bird site. It's not solving a huge problem but not having to stretch my finger to the top of the screen is nice when I'm feeling extra lazy.

Considerations

  • Is soft-reset the right event to listen to here? It seemed the most fitting since it's the event that fires when the tab is already selected.
  • This is a pattern I see often on iOS but can't speak for Android, does it make sense there?
  • The focused state does not get reset across page navigation, is this awkward?
    • Note: It seems that this is also the case if you manually focus the input field.
  • If this is an addition for the lazy among us, would it make sense to also add a more convenient way to blur the input?
    • For example: If there are no search results and the user presses on the page body

@rshigg rshigg changed the title Focus search input on soft reset Focus search input on Search tab soft reset Oct 11, 2024
@mary-ext
Copy link
Contributor

Dupe of #3065 (although it's been a long time since this PR was made)

Yeah, soft-reset is the proper event, are you also checking if you're at the suggestions/explore page?

@rshigg
Copy link
Contributor Author

rshigg commented Oct 12, 2024

Dupe of #3065 (although it's been a long time since this PR was made)

Good callout! I can close this PR if you think it's redundant 👍

Yeah, soft-reset is the proper event, are you also checking if you're at the suggestions/explore page?

To my understanding, the explore page is inside SearchScreenInner which is rendered by the Search screen where I focus the input, so that should have us covered. Unless you mean that we shouldn't focus the input when the explore page is visible?

@Mattis142
Copy link

ik this adds nothing but I just wanted to confirm that yes indeed this is also an interaction that's pretty normal on android 😅

@haileyok
Copy link
Contributor

haileyok commented Nov 3, 2024

Ahh yes! There was something that was blocking when #3065 was originally submitted that I didn't have time to look at, but we've refactored a lot of stuff around this screen and we might be able to do this now. I'll have to run the sim this coming week to see if I can suss out what the problem was last time...

In either case, I see that this change is just a one liner while there's a bit more stuff going on in #3065. Haven't looked closely at either of these yet, but have you tested this one liner against web as well? Looks like thats what some of the extra stuff was addressing in the other PR.

@rshigg
Copy link
Contributor Author

rshigg commented Nov 3, 2024

Ahh yes! There was something that was blocking when #3065 was originally submitted that I didn't have time to look at, but we've refactored a lot of stuff around this screen and we might be able to do this now. I'll have to run the sim this coming week to see if I can suss out what the problem was last time...

Awesome! Really excited to potentially get this merged if all goes well 😁

In either case, I see that this change is just a one liner while there's a bit more stuff going on in #3065. Haven't looked closely at either of these yet, but have you tested this one liner against web as well? Looks like thats what some of the extra stuff was addressing in the other PR.

It only runs on mobile currently, since it felt like a mobile app pattern to me. There's definitely an argument to be made to add it web as well though.

@rshigg
Copy link
Contributor Author

rshigg commented Nov 10, 2024

@haileyok congrats on the 1.93 release! This feature seems to be getting a lot of traction lately, so might be worth potentially merging into the next release? 🤞😄

No rush obviously, I appreciate all the hard work the team is putting in.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

looks good to me. let's do it for native for now, maybe we'll add to web too

@gaearon gaearon merged commit 56a8809 into bluesky-social:main Nov 25, 2024
@hichemfantar
Copy link

looks like this is relevant #6622

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.

6 participants