[MBL-1500] Update and use banners for ppo #2094
Conversation
| .foregroundColor(vm.messageTextColor) | ||
| .padding(EdgeInsets(top: 16, leading: 9, bottom: 16, trailing: 9)) | ||
| .background { | ||
| RoundedRectangle(cornerRadius: 6) |
There was a problem hiding this comment.
This corner radius (and possibly the padding) matches the figma for PPO but is different from the UIKit banners. I'm going to confirm with Alison if we want to match the figma or if we want to be consistent with the app before I submit the pr.
There was a problem hiding this comment.
What do you think about having some kind of MessageBannerViewLayout enum to avoid hardcoding the padding, cornerRadius, minHeight, and spacing values?
There was a problem hiding this comment.
Done! I wanted to do a shared enum between the swiftUI and UIKit versions, but the UIKit version is primarily a storyboard, so I gave up on that pretty quickly... I've also changed some of the values (and re-recorded screenshots) after talking to Alison and determining that PPO styling should match current standards, not our Figma mocks.
| .foregroundColor(vm.messageTextColor) | ||
| .padding(EdgeInsets(top: 16, leading: 9, bottom: 16, trailing: 9)) | ||
| .background { | ||
| RoundedRectangle(cornerRadius: 6) |
There was a problem hiding this comment.
What do you think about having some kind of MessageBannerViewLayout enum to avoid hardcoding the padding, cornerRadius, minHeight, and spacing values?
| @@ -0,0 +1,52 @@ | |||
| @testable import Kickstarter_Framework | |||
| // @testable import KsApi | |||
There was a problem hiding this comment.
looks like we can remove this import for now
There was a problem hiding this comment.
Is the image icon supposed to show in these snapshots? Either the checkmark or "i" info icon?
There was a problem hiding this comment.
No, we're not using an info icon - that's why the info banners are centered instead of aligned to the left. I think it's a bit of a weird choice, but it matches what our other banners do so I'm pretty sure it's intentional. I removed the empty image, though, so it should look better now!
📲 What
Update the swiftUI banners and use a sample one in the PPO view.
Note: I've added a test helper for snapshots of a swiftui view instead of our current view controllers. This doesn't match the format of our current tests (with a lot of different device sizes), but we could easily extend this to add a default iPad frame and a default landscape frame as well.
I do think this way of testing swiftui might make it harder to test different Environments (ex different font sizes or color schemes) but I also don't think it's worth investing a lot of time into that until we start actually working on dark mode. I'm open to suggestions, though!
Also an intentional difference from the way we often test: I'm hardcoding strings here instead of using known strings and testing different languages. The goal of these tests isn't to test our translations, so having a short string and a long string for each layout should be sufficient and makes sure we don't have extraneous snapshots. If I've missed anything/there are other reasons to use translatable strings and test all/multiple languages, let me know!
👀 See
Jira
Also see snapshots!
♿️ Accessibility
✅ Acceptance criteria