Fix #3374: Add opt-in to Brave Today #3407
Conversation
kylehickinson
force-pushed
the
bt-opt-in
branch
2 times, most recently
from
5bd6e80
to
31fc625
Compare
There was a problem hiding this comment.
Looks good, add a comment to a NSLocalized string,
general rule I use is the less words are used in a string, the more explanation it needs
you English speakers have it easy, look at this funny example from my language,
all these words refer to word play, they differ based on sentence and context
BraveShared/BraveStrings.swift
Outdated
| "today.introCardNew", | ||
| bundle: .braveShared, | ||
| value: "New", | ||
| comment: "" |
There was a problem hiding this comment.
Add comment what this new refers to, many languages use different words for it depending on context.
It should mention something like new as new feature introduced
| /// Displays the word "New" with a gradient mask | ||
| private class MaskedNewLabel: UIView { | ||
| private let gradientView = GradientView( | ||
| // colors: [UIColor(rgb: 0xA78AFF), UIColor(rgb: 0xFF1893), UIColor(rgb: 0xFA7250)], // dark |
| @@ -48,7 +48,16 @@ class BraveTodaySettingsViewController: TableViewController { | |||
| dataSource.sections = [ | |||
| .init( | |||
| rows: [ | |||
| .boolRow(title: Strings.BraveToday.isEnabledToggleLabel, option: Preferences.BraveToday.isEnabled) | |||
| .boolRow(title: Strings.BraveToday.isEnabledToggleLabel, option: Preferences.BraveToday.isEnabled, onValueChange: { value in | |||
There was a problem hiding this comment.
Is onValueChange handler safe without weak or unowned self here?
There was a problem hiding this comment.
Hmm, no I think it'll lead to a retain cycle, will add weak/unowned
|
Haha, good call on the localization comments 😆 Will fix those |
kylehickinson
force-pushed
the
bt-opt-in
branch
2 times, most recently
from
29db501
to
104d3c9
Compare
kylehickinson
added
the
blocked
kylehickinson
force-pushed
the
bt-opt-in
branch
from
104d3c9
to
042a5f9
Compare
kylehickinson
removed
the
blocked
Summary of Changes
This pull request fixes #3374
This pull request fixes #3290
Submitter Checklist:
NSLocalizableString()Test Plan:
Brave Today Opt-In Specin DriveScreenshots:
Reviewer Checklist:
QA/(Yes|No)release-notes/(include|exclude)bug/enhancement