-
Notifications
You must be signed in to change notification settings - Fork 3k
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-11132 [Homepage] [Message] initial state + view for message card #24271
Conversation
func configure(state: MessageCardState, theme: Theme) { | ||
applyGleanMessage(with: state.title, description: state.description, buttonLabel: state.buttonLabel) | ||
applyTheme(theme: theme) | ||
ctaButton.applyTheme(theme: theme) | ||
} |
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.
copied most of this class over, mainly changed this method from the original in LegacyHomepageMessageCardCell
. Also moved the shadow constants used in setupShadow
here instead of calling it from HomepageViewModel
Previous implementation
func configure(viewModel: HomepageMessageCardViewModel, theme: Theme) {
self.viewModel = viewModel
if let message = viewModel.getMessage(for: .newTabCard) {
applyGleanMessage(message)
}
applyTheme(theme: theme)
ctaButton.applyTheme(theme: theme)
}
```
Client.app: Coverage: 32.31
Generated by 🚫 Danger Swift against 9ccd9cd |
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.
One small nit but lgtm
@@ -107,6 +109,27 @@ final class HomepageSectionLayoutProvider { | |||
return section | |||
} | |||
|
|||
private func createMessageCardSectionLayout(for traitCollection: UITraitCollection) -> NSCollectionLayoutSection { | |||
let itemSize = NSCollectionLayoutSize(widthDimension: .fractionalWidth(1), | |||
heightDimension: .estimated(180)) |
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.
could we move this value out in to a constant?
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! I was also contemplating on consolidating this logic into one reusable method, since its similar for header section / customize homepage section. I would pass in the constants as params. What do you think?
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.
will merge this PR and discuss on slack + create a separate PR to address this comment!
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.
ok!
📜 Tickets
Jira ticket
Github issue
💡 Description
Add initial view and state for the message card section for the new homepage.
MessageCardState
with mock string📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120
)Screenshot