-
Notifications
You must be signed in to change notification settings - Fork 441
Implement UI for Brave Ads state level targeting #2637
Implement UI for Brave Ads state level targeting #2637
Conversation
It has been decided that state names will not be localized in the MVP |
87d74ff
to
fb52c11
Compare
static let adsSubdivisionTargeting = NSLocalizedString("BraveRewardsAdsSubdivisionTargeting", bundle: .rewardsUI, value: "State level ad targeting", comment: "") | ||
static let adsSubdivisionTargetingTitle = NSLocalizedString("BraveRewardsAdsSubdivisionTargetingTitle", bundle: .rewardsUI, value: "State level ad targeting", comment: "") |
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.
These two strings have the same value. There a reason for having both?
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.
Fixed
cell.label.text = Strings.adsSubdivisionTargeting | ||
|
||
var adsSubdivisionTargetingCode: String | ||
if state.ads.subdivisionTargetingCode == "DISABLED" { |
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.
Is there no const definition of this on ads library side that can be used to avoid string-checking here and L232?
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.
Not at this time, however, I will raise a new issue to add an API call to detect if disabled. So recommended we add at a future date, thanks
bf2011e
to
c2205ac
Compare
c2205ac
to
ea51dec
Compare
Summary of Changes
This pull request fixes #2636
Submitter Checklist:
NSLocalizableString()
Test Plan:
See brave/brave-browser#9200
Screenshots:
Reviewer Checklist:
QA/(Yes|No)
release-notes/(include|exclude)
bug
/enhancement