[NT-1355] Lights On header bugfix #1224
Conversation
| @@ -241,7 +243,7 @@ public final class DiscoveryPageViewModel: DiscoveryPageViewModelType, Discovery | |||
| } | |||
| } | |||
|
|
|||
| self.contentInset = self.viewWillAppearProperty.signal | |||
| self.contentInset = self.viewWillAppearIsEditorialProperty.signal.filter(isFalse) | |||
There was a problem hiding this comment.
Ahhh sorry about that! 🙈 I actually noticed this and thought it always went under the header haha.
The problem is when the native_project_cards experiment is turned on, the the background color needs to be updated so there's enough contrast with the cards :\
It's not terrible, I guess.
Another approach would be to prevent the new project cards from displaying if the cards are within the context of an EditorialViewController.
There was a problem hiding this comment.
I'll go with the latter for now.
| @@ -241,7 +243,7 @@ public final class DiscoveryPageViewModel: DiscoveryPageViewModelType, Discovery | |||
| } | |||
| } | |||
|
|
|||
| self.contentInset = self.viewWillAppearProperty.signal | |||
| self.contentInset = self.viewWillAppearIsEditorialProperty.signal.filter(isFalse) | |||
There was a problem hiding this comment.
Does updating the content inset actually break anything?
There was a problem hiding this comment.
It is updated by the parent when contained in editorial VC.
justinswart
force-pushed
the
NT-1355-editorial-header-bugfix
branch
from
4c9b998 to
c2b8e07
Compare
# Conflicts: # Library/ViewModels/DiscoveryPageViewModel.swift
|
Note this has since been updated to instead provide the background color for discovery from a shared function which is used for the cells and the background of |
| _ = self.view | ||
| |> \.backgroundColor .~ ( | ||
| // Update the background if it is not currently clear (contained in EditorialProjectsViewController) | ||
| self.view.backgroundColor != .clear ? discoveryPageBackgroundColor() : self.view.backgroundColor |
There was a problem hiding this comment.
Unfortunately snapshot tests wouldn't pass without this - it seems setting the backgroundColor any earlier than bindStyles was ineffective. We only ever set the backgroundColor to .clear when this is contained in EditorialProjectsViewController.
|
@jgsamudio good question - we did this for the header originally when we used it for Go Rewardless (#971), but perhaps it doesn't work for this particular image, what do you think, @colleenmacd? |
|
I think the animation doesn't work as well for this image because the image
has a lot of grain/texture on it, which contributes to it looking slightly
pixelated when scaled up. The Go Rewardless image was a PNG graphic
floating on top of a solid background so it worked better. If it's simple
to disable that, I'd vote for that, but if its a larger effort, let me
know— not a big deal to keep it
…On Mon, Jun 15, 2020 at 11:49 AM Justin Swart ***@***.***> wrote:
@jgsamudio <https://github.com/jgsamudio> good question - we did this for
the header originally when we used it for Go Rewardless (#971
<#971>), but perhaps it
doesn't work for this particular image, what do you think, @colleenmacd
<https://github.com/colleenmacd>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1224 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMOMN4AYIMG527VWHKPXB5LRWY7GXANCNFSM4N34FVGQ>
.
--
Colleen MacDonald She/Her
Senior Product Designer, Native Apps
Kickstarter is a Public Benefit Corporation
<https://www.kickstarter.com/charter>
|
📲 What
Fixes a bug causing the project cards to be scrolled behind the Lights On header in the editorial projects collection.
Note: Contains unrelated formatting updates from
bin/format.sh .not being run in #1223 😅🤔 Why
This is a regression introduced where
DiscoveryPageViewController'sbackgroundColoris changed after it's added as a child ofEditorialProjectsViewController. We need this to remain clear for the behaviour in the editorial projects list.🛠 How
When this
DiscoveryPageViewController'sviewWillAppearis called we pass in a boolean to indicate whether its parent isEditorialProjectsViewControllerand we don't emit thebackgroundColororcontentInsetchanges in that case as these are configured by the parent VC.Note: Initially I thought I would just pass the VC's class into the VM for this logic but that would require the VM to be aware of VC classes.
👀 See
✅ Acceptance criteria