[MBL-1209] Select Your Reward Title #1952
Conversation
scottkicks
changed the title
…kstarter/ios-oss into mbl-120/rewards-screen-title
…kstarter/ios-oss into mbl-120/rewards-screen-title
this will get rolled out for all projects so I didn't include inPostCampaignPledgeState checks
scottkicks
changed the title
scottkicks
force-pushed
the
mbl-120/rewards-screen-title
branch
from
79850ad
to
004bfec
Compare
scottkicks
requested review from
a team and
ifosli
and removed request for
a team
|
|
||
| if featurePostCampaignPledgeEnabled() { | ||
| let topSafeAreaInset = self.view.safeAreaInsets.top | ||
| let topInset = self.headerView.frame.height - topSafeAreaInset + Styles.grid(1) |
There was a problem hiding this comment.
I'm a little confused - what's the point of manually calculating the top inset here instead of using auto layout to set the top of the collection view to below the header? I think constraints are generally cleaner, though we'd have to do some truncate to bounds stuff that might complicate it. I'm also a little concerned that manual calculation would do weird things if the user changes the font size (but our app in general does weird things here, so I don't think that needs to be a blocker)
| self.headerView.leftAnchor.constraint(equalTo: self.view.leftAnchor).isActive = true | ||
| self.headerView.rightAnchor.constraint(equalTo: self.view.rightAnchor).isActive = true | ||
| self.headerView.topAnchor.constraint(equalTo: self.view.topAnchor).isActive = true | ||
| self.headerView.widthAnchor.constraint(equalTo: self.view.widthAnchor).isActive = true |
There was a problem hiding this comment.
nit: Does the width anchor do anything when we already have left and right? Also, the height is underconstrained - does it give a build warning? If it does, can you add a height constraint and just set it to the intrinsic content size?
There was a problem hiding this comment.
I was seeing issues originally, but it looks like now that I'm setting the collectionView's topAnchor directly, that issue has been fixed 🤷 .
| private let labelStyle: LabelStyle = { label in | ||
| label | ||
| |> \.numberOfLines .~ 1 | ||
| |> \.lineBreakMode .~ .byTruncatingTail |
There was a problem hiding this comment.
Are you sure we should be truncating this? My general ux experience says that strings we provide should never be truncated. Happy to follow what UX recommends, but please double check to make sure. I really think we shouldn't truncate it at all. (Though it might be worth asking Alison what she thinks the UX should look like in ex german with the largest font size - does adding a title make the screen unusable, especially in landscape mode?)
|
@ifosli I tested landscape mode in a few device sizes but didn't consider testing different languages or larger accessibility fonts. Thanks for calling that out! I think I addressed all of your concerns. Next time, I'll try to remember to add a screen recording of landscape so that it's clear. |
📲 What
Adds "Select your reward." to Rewards views if post campaign feature flag is on
🤔 Why
With the new PCP flow having an additional "Confirm details" screen, design has decided that it'd be beneficial to title each page to provide users with more context.
🛠 How
Adds a header to the RewardsCollectionView.
Note that there is a touch more space between the rewards options and the title than the designs suggest. This is because each reward moves up slightly when scrolling through their details. Designs didn't catch that edge case and will update the figma soon.
The title string is hardcoded for now and will need to be updated once all copy translations are in. I've added a comment with the ticket to address that.
👀 See
✅ Acceptance criteria