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
Add FXIOS-9027 Integrate address toolbar in Client #20142
Conversation
…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
case tap | ||
case longPress | ||
} | ||
|
||
class ToolbarButton: UIButton, ThemeApplicable { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
firefox-ios/Client/Frontend/Browser/Toolbars/NavigationToolbarContainer.swift
Show resolved
Hide resolved
case didLoadToolbars | ||
} | ||
|
||
class ToolbarMiddlewareAction: Action { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swift
Outdated
Show resolved
Hide resolved
import Redux | ||
import ToolbarKit | ||
|
||
class ToolbarMiddleware: FeatureFlaggable { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This pull request has conflicts when rebasing. Could you fix it @thatswinnie? 🙏 |
...tend/Browser/BrowserViewController/Extensions/BrowserViewController+TabToolbarDelegate.swift
Outdated
Show resolved
Hide resolved
…lient # Conflicts: # firefox-ios/Client/Nimbus/NimbusFeatureFlagLayer.swift # firefox-ios/nimbus.fml.yaml
…ate-address-toolbar-in-client
…ate-address-toolbar-in-client
Client.app: Coverage: 31.11
Generated by 🚫 Danger Swift against 6b32cba |
📜 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
@Mergifyio backport release/v120
)