-
Notifications
You must be signed in to change notification settings - Fork 328
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
[Customer Center] Slight improvement to the Customer Center Promotional Offer view #4554
Conversation
@vegaro genuinely not sure what the labels should be - it's customer center but not really a feature, went with "other" |
@@ -110,7 +143,6 @@ struct PromotionalOfferHeaderView: View { | |||
private(set) var viewModel: PromotionalOfferViewModel | |||
|
|||
private let spacing: CGFloat = 30 | |||
private let topPadding: CGFloat = 150 |
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.
moved the padding from here to the icon view so it can be controlled from there
private enum AppIconHelper { | ||
static func getAppIcon() -> UIImage? { | ||
guard let iconsDictionary = Bundle.main.infoDictionary?["CFBundleIcons"] as? [String: Any], | ||
let primaryIcons = iconsDictionary["CFBundlePrimaryIcon"] as? [String: Any], | ||
let iconFiles = primaryIcons["CFBundleIconFiles"] as? [String], | ||
let lastIcon = iconFiles.last else { | ||
return nil | ||
} | ||
return UIImage(named: lastIcon) |
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 fetch the app icon, seems to work well in my testing
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.
🤔 looks a bit blurry actually, maybe I can do something to ensure we're always getting the highest-res version
@@ -62,6 +62,12 @@ struct PromotionalOfferView: View { | |||
|
|||
VStack { | |||
if self.viewModel.error == nil { | |||
|
|||
AppIconView() | |||
.padding(.top, 100) |
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.
Since this padding is always applied, the padding will still be present even if the AppIconView() is just an EmptyView(), which could make the layout look off if the app icon can't be found for some reason. I'd consider making a static property on AppIconView called canDisplay
and then conditionally setting the top padding based on that value, like:
.padding(.top, AppIconView.canDisplay ? 100 : 0)
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.
Oh, I think you switched the missing case from being an EmptyView() to a Color.clear while I was reviewing. Ignore this comment since we want to preserve the space!
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.
yeah sorry for the switcheroo, I had uncommitted code. I guess I'm starting to forget how to do PRs properly
🤔 actually I think we might not wanna merge this and instead have the image in the backend unfortunately. The image is blurred, since the only icon that is actually embedded in the app is the size that the app would need. So I just get a 60x60 icon, doesn't look great |
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.
I love it. It takes de screen to the next level
The promo offer view felt pretty barren. This is a very small update, but I feel like it looks significantly better this way.
All I'm doing is reading the icon from the bundle if available. If the image cannot be loaded, it looks exactly the same as it used to.