-
Notifications
You must be signed in to change notification settings - Fork 5
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
[MOB 1953] Separate search and referral impact #568
[MOB 1953] Separate search and referral impact #568
Conversation
let info = ClimateImpactInfo.referral(value: User.shared.referrals.impact, | ||
invites: User.shared.referrals.count) | ||
let view = NTPImpactRowView(info: info) | ||
view.info = info // Needed to force info setup after init |
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.
Interested to get more insights on this one. Is it strictly needed?
I tried to put a breakpoint in the didSet
of that var info: ClimateImpactInfo
and seems like it gets there even without re-assigning the value here.
Also, if the intent is to reassign after the init, I was wondering whether something like this 👇 in the NTPImpactRowView.init
would work as well:
defer {
self.info = info
}
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.
It unfortunately is, this basically resulted from the fact that I prepared the view to automatically refresh every time the info changes. It initially was not even passed on init (mimicking the behaviour of a cell that is configured afterwards), but I had to pass it solely to support the existence of the progress bar.
You are right that didSet
is in fact already called, but for some reason that is not the correct point in the lifecycle for it to properly update. defer
is an interesting suggestion and should in fact work, I can try it since it seems less error prone 👍
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.
It also works with the defer
, just updated it, thanks for the suggestion ❤️
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.
LGTM 🟢
…rch_and_referral_impact
@d4r1091 had to fix a merge conflict, can you check again for another 👍 ? |
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.
Another 👍
MOB-1953
Context
We'll no longer have one global counter to rule them all, we'll instead separate into search and referral impact counters.
Core-side counterpart: https://github.com/ecosia/ios-core/pull/115
Approach
User.shared.searchImpact
instead ofUser.shared.impact
ClimateImpactInfo
so it is more clear the existence of two separate impact counterUser.shared.referrals.count
and noneUser.shared.referrals.impact
)