-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Privacy mode as default #6904
Privacy mode as default #6904
Conversation
eb5b88c
to
fc36df7
Compare
9e2e970
to
2b24a3e
Compare
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.
Only one minor requested change right now. I need to more carefully review the changes to app/scripts/ui.js
and app/scripts/controllers/provider-approval.js
. It seems to make sense, but I want to do another pass before final approval.
app/scripts/ui.js
Outdated
} | ||
|
||
/** | ||
* Establishes streamed connections to background scripts and a Web3 provider |
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.
Everything below here is just moved from popup-core.js
correct?
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.
In the separate notification bundle PR, I did essentially the opposite of this: I moved more things from ui.js
into popup-core.js
, because they were shared between ui.js
and the newly created notification.js
.
I'm guessing they were merged here because the separation wasn't useful? It will shortly become useful, so it might be worth moving everything back to popup-core.js
. Or you can not bother, and I'll just do that when resolving this conflict in my PR - I don't mind.
Oh and you got rid of async
too! 😅 That's funny, I did the same thing.
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.
Everything below here is just moved from
popup-core.js
correct?
Yes, that file was only used here so I merged it/inlined it. The name wasn't quite accurate and it only had this single use.
I think this diff and your diff re-introducing separate files might be good together (that is, I'll leave this for now). It seems, er, semantically correct to me that we'd split things out later if needed.
import { compose } from 'recompose' | ||
import BackupNotification from './home-notification.component' | ||
|
||
const mapDispatchToProps = () => { |
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 this empty mapDispatchToProps necessary?
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 is not, good catch!
ui/app/pages/home/home.component.js
Outdated
<TransactionView> | ||
{ | ||
showPrivacyModeNotification && ( | ||
<HomeNotification |
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.
So if I read this right, this notification is only meant to be shown once, correct? And that is after the migration?
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.
Yes, once and only once
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.
Okay, everything looks good to me. I've left a couple questions and we should make a decision on the comment from Mark. If this passes QA, I will approve.
@bdresser Tagging you as a reviewer as you probably have the sharpest sense of how this should behave, so I think a QA from you would be good. Also, @tmashuang, if you could test that it behaves as expected in the case of a user who has privacy mode off before this update is made, that would be good. |
665727f
to
dedc64a
Compare
The notification component styles should be consistent with the notification references in this issue #6874 I'm noticing that there's an opaque bg-color applied to the button that is giving it a darker color than it's parent container. Check out this pen to see how the styles are intended to be implemented https://codepen.io/cjeria/pen/mNmEYj |
d08a245
to
a436294
Compare
Yes, @danjm and I are aware that the two are slightly different and we'll need to merge both their implementation and design when both PRs are landed on
Subtle catch, thanks. Fixed. |
a436294
to
040490e
Compare
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
Closes #6352, fixes #6320 and fixes #6110
This PR sets Privacy Mode to be "on"/enabled by default, leaving the toggle in settings to allow folks to disable it wholly and return the to old behaviour.
Components
What does the migration notice look like
Users for who Privacy Mode was previous disabled will see:
What does the notification look like
The design: screenshot, and Figma
As implemented: