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

Bugfix FXIOS-10696 Dismiss search bar when tap on the outside view in HistoryPanel #23276

Merged

Conversation

gokulvenkat243
Copy link
Contributor

@gokulvenkat243 gokulvenkat243 commented Nov 21, 2024

📜 Tickets

Jira ticket
Github issue

💡 Description

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@ih-codes
Copy link
Collaborator

Hi @gokulvenkat243! Thank you for your contribution. 🙏

I took a quick look at your PR. Your code does helpfully close the HistoryPanel's search bar, but I think we should also account for closing the keyboard with this solution (see video below for the problem).

You can try calling searchbar.resignFirstResponder() instead of calling it on the bottomStackView. You can test in the simulator by making sure you have the software keyboard enabled.

RPReplay_Final1732217854.MP4

Copy link
Collaborator

@ih-codes ih-codes left a comment

Choose a reason for hiding this comment

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

Thanks @gokulvenkat243 ! Looks good.

I will get this merged in on Monday. 👍

@gokulvenkat243 gokulvenkat243 changed the title dismiss search bar when tap on the outside view in HistoryPanel Add FXIOS-#23355 - Dismiss search bar when tap on the outside view in HistoryPanel Nov 25, 2024
@gokulvenkat243 gokulvenkat243 changed the title Add FXIOS-#23355 - Dismiss search bar when tap on the outside view in HistoryPanel Bugfix FXIOS-#23355 - Dismiss search bar when tap on the outside view in HistoryPanel Nov 25, 2024
@ih-codes
Copy link
Collaborator

Just closing briefly to get Bitrise to update.

@ih-codes ih-codes closed this Nov 25, 2024
@ih-codes ih-codes reopened this Nov 25, 2024
@ih-codes ih-codes changed the title Bugfix FXIOS-#23355 - Dismiss search bar when tap on the outside view in HistoryPanel Bugfix FXIOS-10696 Dismiss search bar when tap on the outside view in HistoryPanel Nov 25, 2024
@ih-codes
Copy link
Collaborator

Some unrelated CI tests are failing in the pipeline. @gokulvenkat243 Can you please rebase on the latest main for Bitrise? 🙏 Thanks!

@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented Nov 26, 2024

Messages
📖 Project coverage: 33.72%
📖 Edited 2 files
📖 Created 0 files

Client.app: Coverage: 31.63

File Coverage
HistoryPanel.swift 34.8% ⚠️
HistoryPanel+Search.swift 14.46% ⚠️

Generated by 🚫 Danger Swift against 8c3cf1d

@ih-codes
Copy link
Collaborator

Hi @gokulvenkat243, I was just chatting with @FilippoZazzeroni who is more familiar with this area of the code. He would like to approach this fix in a better way using the UISearchBarDelegate. This is because your new gesture only works if you have an empty history panel. Otherwise, tapping on a row will cause you to visit the webpage, not close the keyboard.

If you add this line to the UISearchBar:

lazy var searchbar: UISearchBar = .build { searchbar in
        searchbar.searchTextField.placeholder = self.viewModel.searchHistoryPlaceholder
        searchbar.returnKeyType = .go
        searchbar.delegate = self
        searchbar.showsCancelButton = true // <---- Add this line
    }

and inside the HistoryPanel+Search.swift file, you can implement the following delegate method with your code instead:

    func searchBarCancelButtonClicked(_ searchBar: UISearchBar) {
        bottomStackView.isHidden = true
        searchBar.resignFirstResponder()
    }

@ih-codes
Copy link
Collaborator

Thanks for that change! You can also safely remove your tab gesture code now, I believe the cancel button will accomplish the same need. 🙏

@ih-codes ih-codes merged commit 65b7f9a into mozilla-mobile:main Nov 26, 2024
6 checks passed
@ih-codes
Copy link
Collaborator

Thanks @gokulvenkat243 for your changes, this has been merged! 🥳

@gokulvenkat243
Copy link
Contributor Author

Thanks for your help @ih-codes ❤️

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.

3 participants