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-9027 Integrate address toolbar in Client #20142

Merged
merged 30 commits into from May 13, 2024

Conversation

thatswinnie
Copy link
Contributor

📜 Tickets

Jira ticket
Github issue

💡 Description

This integrates the navigation toolbar with dummy buttons - only the home button works so far.

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

…lient

# Conflicts:
#	firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/GeneralBrowserAction.swift
#	firefox-ios/Client/Frontend/Browser/BrowserViewController/State/BrowserViewControllerState.swift
#	firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift
…lient

# Conflicts:
#	firefox-ios/Client/Frontend/Browser/BrowserViewController/State/BrowserViewControllerState.swift
@thatswinnie thatswinnie requested a review from OrlaM May 3, 2024 15:56
@thatswinnie thatswinnie requested a review from a team as a code owner May 3, 2024 15:56
case tap
case longPress
}

class ToolbarButton: UIButton, ThemeApplicable {
Copy link
Contributor

Choose a reason for hiding this comment

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

final ?

case goToHomepage
}

class GeneralBrowserMiddlewareAction: Action {
Copy link
Contributor

Choose a reason for hiding this comment

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

final ?

setupLayout()
}

required init?(coder: NSCoder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@available(*, unavailable)

import Redux
import UIKit

class NavigationToolbarContainer: UIView, ThemeApplicable, StoreSubscriber {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

case didLoadToolbars
}

class ToolbarMiddlewareAction: Action {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

import Redux
import ToolbarKit

class ToolbarAction: Action {
Copy link
Contributor

Choose a reason for hiding this comment

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

final

import Redux
import ToolbarKit

class ToolbarMiddleware: FeatureFlaggable {
Copy link
Contributor

@Ramiz69 Ramiz69 May 5, 2024

Choose a reason for hiding this comment

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

you should use final everywhere where it is needed, since you forcefully enable static dispatching and you or other developers immediately understand that this object has no child and do not have to worry about the fact that your edits may break something somewhere else in code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment. We only use final in rare cases in our project when we want to indicate that this class should not be subclassed. In most cases adding final is therefor not necessary.

Copy link
Contributor

mergify bot commented May 9, 2024

This pull request has conflicts when rebasing. Could you fix it @thatswinnie? 🙏

…lient

# Conflicts:
#	firefox-ios/Client/Nimbus/NimbusFeatureFlagLayer.swift
#	firefox-ios/nimbus.fml.yaml
@mobiletest-ci-bot
Copy link

Warnings
⚠️ Variable 'crossCircleFill' in Medium is out of alphabetical order.
⚠️ Variable 'cross' in Medium is out of alphabetical order.
Messages
📖 Project coverage: 32.58%
📖 Edited 11 files
📖 Created 7 files

Client.app: Coverage: 31.11

File Coverage
BrowserViewController+TabToolbarDelegate.swift 11.66% ⚠️
GeneralBrowserAction.swift 0.0% ⚠️
BrowserViewController.swift 4.5% ⚠️
NavigationToolbarContainer.swift 0.0% ⚠️
BrowserViewControllerState.swift 0.0% ⚠️
AppState.swift 89.66%
ToolbarFlagManager.swift 0.0% ⚠️
ToolbarState.swift 0.0% ⚠️
ToolbarAction.swift 0.0% ⚠️
NimbusFlaggableFeature.swift 97.78%
NimbusFeatureFlagLayer.swift 73.49%
ToolbarMiddleware.swift 11.76% ⚠️
NavigationToolbarContainerModel.swift 0.0% ⚠️

Generated by 🚫 Danger Swift against 6b32cba

@thatswinnie thatswinnie merged commit a111004 into main May 13, 2024
12 of 13 checks passed
@thatswinnie thatswinnie deleted the wt/FXIOS-9027-integrate-address-toolbar-in-client branch May 13, 2024 14:55
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.

None yet

5 participants