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

Autofocus location search bar when switching tabs #6832

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MrChocolatine
Copy link
Contributor

@MrChocolatine MrChocolatine commented Sep 21, 2024

What this PR changes

This PR adds an autofocus to the search bar, of the Selection location view, when changing tabs.

Why this is wanted

To contribute and implement the feature requested in #6554 .
(I know I was told the UI is going to be re-worked: #6554 (comment) , but I thought I could satisfy my curiosity by giving it a shot)

How it's implemented

While I know React, I am not pro-efficient with it as it's not my daily tool. So I hope my implementation is okay-ish and fits your standards.

By notifying when the location type has changed (using useBoolean()), while passing the information to the <SearchBar> component.

How to test the change

The autofocus of the search bar is currently not tested, even when it was initially added:
d5f216e (#4163)


Fixes #6554


This change is Reviewable

@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from 93efc62 to bdfff40 Compare September 21, 2024 16:02
@MrChocolatine MrChocolatine marked this pull request as ready for review September 21, 2024 16:03
@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from bdfff40 to 293ddab Compare September 22, 2024 10:41
@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from 293ddab to 47f69da Compare September 23, 2024 09:03
@raksooo
Copy link
Member

raksooo commented Sep 24, 2024

Thank you for providing us with this work! It's definitely a nice addition! I have some concerns about the code though:

  1. The SearchBar component is used in the Split tunneling view as well and in there the LocationType-props doesn't make much sense. I would prefer to not pass anything that specific to that component.
  2. The useBoolean approach seems a bit more complicated than needed, but should work fine.

I have some suggestions for you if you would like to improve it yourself, or if you want me to do it I can submit another PR for this :) Here is how I would do it:

  • Forward a ref into SearchBar using React.forwardRef and combine it with the inputRef using useCombinedRefs. You can check AutoResizingTextInputWithRef for inspiration.
  • Keep a ref in the SelectLocation component and pass it down to SearchBar
  • In changeLocationType in SelectLocation call .focus() on the ref.

This would let the SelectLocation component call .focus() on the input field without exposing any internals of the select location view to the search bar. Let me know if you want me to make these changes :)

@MrChocolatine
Copy link
Contributor Author

Thank you for providing us with this work! It's definitely a nice addition! I have some concerns about the code though:

  1. The SearchBar component is used in the Split tunneling view as well and in there the LocationType-props doesn't make much sense. I would prefer to not pass anything that specific to that component.
  2. The useBoolean approach seems a bit more complicated than needed, but should work fine.

I have some suggestions for you if you would like to improve it yourself, or if you want me to do it I can submit another PR for this :) Here is how I would do it:

  • Forward a ref into SearchBar using React.forwardRef and combine it with the inputRef using useCombinedRefs. You can check AutoResizingTextInputWithRef for inspiration.
  • Keep a ref in the SelectLocation component and pass it down to SearchBar
  • In changeLocationType in SelectLocation call .focus() on the ref.

This would let the SelectLocation component call .focus() on the input field without exposing any internals of the select location view to the search bar. Let me know if you want me to make these changes :)

Thanks for the feedback @raksooo .

I will try to rework the PR following your suggestions 👍 .

@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from 47f69da to eb0169f Compare September 25, 2024 09:22
@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch 2 times, most recently from ae9dfbc to 05590d2 Compare October 13, 2024 13:29
@MrChocolatine
Copy link
Contributor Author

Thank you for providing us with this work! It's definitely a nice addition! I have some concerns about the code though:

  1. The SearchBar component is used in the Split tunneling view as well and in there the LocationType-props doesn't make much sense. I would prefer to not pass anything that specific to that component.
  2. The useBoolean approach seems a bit more complicated than needed, but should work fine.

I have some suggestions for you if you would like to improve it yourself, or if you want me to do it I can submit another PR for this :) Here is how I would do it:

  • Forward a ref into SearchBar using React.forwardRef and combine it with the inputRef using useCombinedRefs. You can check AutoResizingTextInputWithRef for inspiration.
  • Keep a ref in the SelectLocation component and pass it down to SearchBar
  • In changeLocationType in SelectLocation call .focus() on the ref.

This would let the SelectLocation component call .focus() on the input field without exposing any internals of the select location view to the search bar. Let me know if you want me to make these changes :)

@raksooo , changes applied following your suggestion ✌️ .

@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch 2 times, most recently from 1b1217e to 3490e35 Compare October 13, 2024 13:34
@@ -67,6 +67,7 @@ export const StyledClearIcon = styled(ImageView)({
});

interface ISearchBarProps {
searchInputRef?: React.Ref<HTMLInputElement>;
Copy link
Contributor Author

@MrChocolatine MrChocolatine Oct 13, 2024

Choose a reason for hiding this comment

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

In your suggestion @raksooo , you showed me AutoSizingTextInputWithRef as an example:

function AutoSizingTextInputWithRef(props: IInputProps, forwardedRef: React.Ref<HTMLInputElement>) {

which consumes a forwardedRef parameter, the ref object does not come from its props.

But here I was only able to pass it via a prop.

Copy link
Member

Choose a reason for hiding this comment

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

To use the ref-prop you will need to wrap the component in React.forwardRef.

function SearchBar(props: ISearchBarProps, forwardedRef: React.Ref<HTMLInputElement>) { ... }

export default React.forwardRef(SearchBar);

Then you won't have to include it in props and you'll be able to use SearchBar like this:

<SearchBar ref={searchInputRef} ... />

I think React.forwardRef won't be needed in future versions of react, but for now we'll need to add it :)

@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from 3490e35 to f7cb189 Compare October 13, 2024 13:41
@MrChocolatine MrChocolatine force-pushed the add-autofocus-on-location-type-change branch from f7cb189 to eb45c2e Compare October 13, 2024 13:43
@MrChocolatine
Copy link
Contributor Author

Strangely tests are failing:

https://github.com/mullvad/mullvadvpn-app/actions/runs/11314889729/job/31465392999#step:11:57
image

while I cannot reproduce locally:

image

@raksooo
Copy link
Member

raksooo commented Oct 14, 2024

The failing test should be fixed in this PR: #6970 :)

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.

[Feature request] Selecting the Entry and Exit list of servers should focus their search input
2 participants