[MBL-3167] Video Feed Bottom Overlay Components #2824
[MBL-3167] Video Feed Bottom Overlay Components #2824scottkicks wants to merge 12 commits intomainfrom
Conversation
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
I have a few requested changes on VideoFeedCell (namely using contentConfiguration). These rest are non-blocking suggestions.
| lightMode: .gray_550, | ||
| lightModeAlpha: 0.24, | ||
| darkMode: .gray_550, | ||
| darkModeAlpha: 0.25 |
There was a problem hiding this comment.
Is it intentional that the lightModeAlpha and darkModeAlpha are different by 0.01?
There was a problem hiding this comment.
Love a screenshot test 👍 Is it possible to set up the test so that the overlay has something dark behind it? It's a bit hard to see, as-is. (But not a blocker.)
| height: device.deviceSize.height | ||
| )) | ||
|
|
||
| cell.configureWith(value: .init( |
There was a problem hiding this comment.
Nit: I find it easier to read if the initializer is referenced by name, like
| cell.configureWith(value: .init( | |
| cell.configureWith(value: VideoFeedItem( |
|
|
||
| assertSnapshot( | ||
| matching: cell, | ||
| as: .image(perceptualPrecision: 0.98), |
There was a problem hiding this comment.
Nit: Does it need the lower precision to work?
| static let reuseIdentifier = "VideoFeedCell" | ||
|
|
||
| private let titleLabel = UILabel() | ||
| private var overlayHostingController: UIHostingController<VideoFeedOverlayView>? |
There was a problem hiding this comment.
This cell should use self.contentConfiguration instead of embedding a UIHostingController. Having embedded view controllers can be finicky to get working consistently, especially with cell recycling.
| @@ -2,5 +2,22 @@ import Foundation | |||
|
|
|||
| public struct VideoFeedItem { | |||
There was a problem hiding this comment.
Nice to see this as a simple plain-ol'-swift struct.
|
|
||
| var body: some View { | ||
| HStack(spacing: Constants.iconSpacing) { | ||
| Image(self.icon) |
There was a problem hiding this comment.
This should use Library.imageNamed(self.icon) instead of creating an image directly from the string. We do some bundle magic in our tests to get images working - changing this will make the pill icons show up in your screenshot tests.
There was a problem hiding this comment.
Huh i'm not seeing imageNamed in the codebase anywhere
There was a problem hiding this comment.
Whoops, I told you the wrong name. It's Library.image(named:). See: https://github.com/kickstarter/ios-oss/blob/main/Library/Sources/Library/Library/Image.swift#L3-L9
| FeedPillView(icon: "video-feed-clock-icon", text: self.item.secondaryPillText) | ||
| } | ||
| .accessibilityElement(children: .combine) | ||
| .accessibilityLabel("\(self.item.categoryPillText), \(self.item.secondaryPillText)") |
There was a problem hiding this comment.
Is there a reason the label is applied to the HStack and not to the individual pills?
There was a problem hiding this comment.
Its just my preference for VoiceOver. Like this it'll read as one "Film, 23 days left" which I feel like is a smoother experience than having a pause in between the two. I'm open to changing it since we don't have a defined VoiceOver pattern for things like this.
There was a problem hiding this comment.
That makes sense - can you throw that in a comment?
| } | ||
|
|
||
| /// Custom frosted glass background view. | ||
| private struct FrostedGlassBackground: UIViewRepresentable { |
There was a problem hiding this comment.
Do we need a custom glass effect? Can we use glassEffect(in:) instead?
There was a problem hiding this comment.
glassEffect is only available in ios 26, but I could check how it looks compared to the designs and add a version conditional
There was a problem hiding this comment.
Ok yeah glass effect doesn't offer much flexibility in terms of adjusting the amount of "frost", and by itself its a way too frosty compared to the designs, so I think a simple custom FrostedGlassBackground like this is the way to go
There was a problem hiding this comment.
Roger that. Two thoughts:
- Should
FrostedGlassBackgroundbepublicand in its own file, so we can reuse it? - Can you throw this extra context in a comment?
There was a problem hiding this comment.
Yea! My plan is to iteratively make changes like this as needed in this feature. I'll end up reusing it for the video overlay controls, so making it public will be the move, but I tend to prefer keeping each PR contained within the context of its changes, if that makes sense.
I'll definitely add a context comment
| ZStack(alignment: .bottom) { | ||
| self.topGradient | ||
| .ignoresSafeArea() | ||
| /// Hidden so VoiceOver jumps straight to the interactive overlay. |
There was a problem hiding this comment.
I like this comment
SwiftLint found issuesWarnings
Generated by 🚫 Danger |
f622180 to
d390033
Compare
📲 What
Adds a static bottom overlay components SwiftUI view to the VideoFeedViewController.
Includes all components from the designs except for the percent funded circle. The progress bar also isn't wire up to anything yet.
🤔 Why
This is where most of the campaign info will be
🛠 How
Building all overlay components via SwiftUI. Reminder that the main
VideoFeedViewControlleris aUIViewControllerto help manage the video players later on. Found that SwiftUI wasn't the best at that in my SPIKE.VideoFeedOverlayViewview and add aVideoFeedBottomOverlayViewto it.VideoFeedCell(managed by the collection view inVideoFeedViewController.👀 See
Trello, screenshots, external resources?
♿️ Accessibility
✅ Acceptance criteria