-
Notifications
You must be signed in to change notification settings - Fork 869
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
Fix NTP Stats Toggle #14839
Fix NTP Stats Toggle #14839
Conversation
A Storybook has been deployed to preview UI for the latest push |
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. Works well!
@@ -27,7 +27,7 @@ export class PrefHookManager<T> { | |||
getInitialValue().then(this.notifyListeners) | |||
} | |||
|
|||
notifyListeners (prefs: T) { | |||
notifyListeners = (prefs: T) => { |
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.
Whoa, it is my understanding that, when this was called by getInitialValue()
, this
wasn't bound and caused failure. And you fixed this by using arrow function as this
in arrow function is who defines it, in this case PrefHookMananger<t>
. Am I right? Interesting!
So do you usually prefer this methodology than bind
? To me, using arrow function looks more handy and robust.
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.
Yeah, exactly 😄 Generally, I prefer arrow functions because they're easier to reason about but they do have a couple of downsides when used with classes. For example, if you use an arrow function on a class, each instance of the class will have a separate function, whereas with a traditional JS function they'll be the same instance.
class FuncClass {
myFunc() {}
}
class ArrowClass {
myFunc = () => {}
}
console.log(new FuncClass().myFunc === new FuncClass().myFunc) // true
console.log(new ArrowClass().myFunc === new ArrowClass().myFunc) // false
but if we bind we get the same result anyway 😆
Resolves brave/brave-browser#24985
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: