-
Notifications
You must be signed in to change notification settings - Fork 973
Follow up for sites splits PR #10296 #10310
Conversation
eda3726
to
4bb21ee
Compare
4bb21ee
to
0cd4d5c
Compare
Resolves brave#10296 Resolves brave#10348 Resolves brave#10310 Auditors: Test Plan:
0cd4d5c
to
8bdd858
Compare
@NejcZdovc I'm going to close as stale for now please re-open when you have a window. Thanks! |
8bdd858
to
f1a68a8
Compare
f1a68a8
to
f21ef48
Compare
840342a
to
1ff4d20
Compare
1ff4d20
to
6aa99fb
Compare
app/common/lib/historyUtil.js
Outdated
return Immutable.List() | ||
} | ||
|
||
sites = makeImmutable(sites) ? makeImmutable(sites).toList() : Immutable.List() |
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.
I know this logic is the same as before, but I think it's wrong. Should it be:
sites = sites ? makeImmutable(sites).toList() : Immutable.List()
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.
(makeImmutable won't return falsey values basically)
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.
Since the above is checking == null (which should also return true for undefined, right?), I think we can simply make it
sites = makeImmutable(sites).toList()
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
6aa99fb
to
ae7b30b
Compare
Resolves brave#10296 Resolves brave#12378 Auditors: Test Plan:
ae7b30b
to
b112310
Compare
|
||
sites = makeImmutable(sites) | ||
|
||
if (!isList(sites)) { |
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.
😄 👌
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.
++ Looks great!
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.
++
Follow up for sites splits PR #10296
Follow up for sites splits PR #10296
Submitter Checklist:
git rebase -i
to squash commits (if needed).Resolves #10296
Auditors:
Test Plan: covered with automated tests
Reviewer Checklist:
Tests