-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-10318 [Menu] Populate account header with data & redux #22718
base: main
Are you sure you want to change the base?
Conversation
This pull request has conflicts when rebasing. Could you fix it @dicarobinho? 🙏 |
@@ -178,6 +190,24 @@ public final class HeaderView: UIView, ThemeApplicable { | |||
titleLabel.text = title | |||
} | |||
|
|||
public func setupDetails(subtitle: String, title: String, icon: UIImage?, warningIcon: String?, theme: Theme) { | |||
titleLabel.font = FXFontStyles.Regular.body.scaledFont() |
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.
This should probably be in the title label setup, as it doesn't change, right?
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.
Yes, so body can be by default... will move it to the label setup.
firefox-ios/Client/Frontend/Browser/MainMenu/Redux/MainMenuState.swift
Outdated
Show resolved
Hide resolved
firefox-ios/Client/Frontend/Browser/MainMenu/Redux/MainMenuState.swift
Outdated
Show resolved
Hide resolved
let title: String = { | ||
if needsReAuth { | ||
return .MainMenu.Account.SyncErrorTitle | ||
} | ||
return userProfile.displayName ?? userProfile.email | ||
}() |
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.
let title: String = { | |
if needsReAuth { | |
return .MainMenu.Account.SyncErrorTitle | |
} | |
return userProfile.displayName ?? userProfile.email | |
}() | |
let title: String = if needsReAuth { | |
return .MainMenu.Account.SyncErrorTitle | |
} else { | |
return userProfile.displayName ?? userProfile.email | |
} |
An alternative; but if you use this, please indent with Xcode because I just copy pasted here. No strong opinions on which to use, just pointing out the option.
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.
Just got some errors after changed the code, maybe something else should be changed. Lets keep this, as it is.
firefox-ios/Client/Frontend/Browser/MainMenu/Views/MainMenuViewController.swift
Show resolved
Hide resolved
…te.swift Co-authored-by: roux g. buciu <[email protected]>
…te.swift Co-authored-by: roux g. buciu <[email protected]>
# Conflicts: # firefox-ios/Client/Coordinators/Browser/BrowserCoordinator.swift # firefox-ios/Client/Frontend/Browser/MainMenu/MainMenuCoordinator.swift # firefox-ios/Client/Frontend/Browser/MainMenu/Redux/MenuNavigationDestination.swift
Client.app: Coverage: 30.52
ComponentLibrary: Coverage: 30.97
Generated by 🚫 Danger Swift against 4d9b292 |
@@ -35,6 +35,8 @@ enum MainMenuActionType: ActionType { | |||
case tapShowDetailsView | |||
case tapToggleUserAgent | |||
case updateCurrentTabInfo | |||
case closeMenu |
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.
These have been removed/renamed in a previous PR. You'll have to remove them and fix with the appropriate updated names (tapCloseMenu
and tapNavigateToDestination
)
This pull request has conflicts when rebasing. Could you fix it @dicarobinho? 🙏 |
# Conflicts: # firefox-ios/Client/Frontend/Browser/MainMenu/MainMenuCoordinator.swift # firefox-ios/Client/Frontend/Browser/MainMenu/Redux/MenuNavigationDestination.swift
Also, when you're refactoring this, like we talked about, can you fix the padding on the header too? Kill two birds with one stone! :D |
📜 Tickets
Jira ticket
Github issue
💡 Description
Implemented the functionality for the Menu account header
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)