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

Add FXIOS-10462 Enhanced Tracking Protection Connection Status Refresh #23052

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petruSt24
Copy link
Collaborator

📜 Tickets

Jira ticket
Github issue

💡 Description

Remade the previous PR after a bad rebase, added only the refresh action in this one but I have another PR that handles the back/dismiss navigation in the BrowserViewController

📝 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)

@petruSt24 petruSt24 requested a review from a team as a code owner November 12, 2024 14:52
@cyndichin
Copy link
Contributor

thanks @petruSt24! I believe this PR also has the navigation logic added as well, not sure if that was intentional or if it will be updated to only include refresh logic. et me know if this PR is still ready for review. If so, my comment from the previous PR applies here too. #22899

@petruSt24
Copy link
Collaborator Author

@cyndichin yes this will be updated to have the navigation based on the example you provided, but will do it in another PR as I'll still need the new actions from this one

Comment on lines +192 to +202
if let navigateTo = state.navigateTo {
switch navigateTo {
case .home:
navigationController?.popToRootViewController(animated: true)
case .back:
navigationController?.popViewController(animated: true)
case .close:
enhancedTrackingProtectionMenuDelegate?.didFinish()
case .settings:
enhancedTrackingProtectionMenuDelegate?.settingsOpenPage(settings: .contentBlocker)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

since this will be addressed in a separate PR, should we put a TODO instead here instead of implementing the navigation logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would add a TODO but removing the navigation code would render the navigation bar unresponsive until the other PR is merged, is that preferred?

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