-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #18262 - [Credit cards] Turn the "Sync cards across devices" button into a "Sync cards" toggle #19207
For #18262 - [Credit cards] Turn the "Sync cards across devices" button into a "Sync cards" toggle #19207
Conversation
This pull request has conflicts when rebasing. Could you fix it @codrut-topliceanu? 🙏 |
Codecov Report
@@ Coverage Diff @@
## master #19207 +/- ##
=========================================
Coverage 34.62% 34.62%
+ Complexity 1614 1613 -1
=========================================
Files 540 541 +1
Lines 21819 21828 +9
Branches 3251 3253 +2
=========================================
+ Hits 7554 7558 +4
- Misses 13367 13370 +3
- Partials 898 900 +2
Continue to review full report at Codecov.
|
2d68358
to
dee08de
Compare
app/src/main/java/org/mozilla/fenix/settings/SyncPreferenceView.kt
Outdated
Show resolved
Hide resolved
dee08de
to
849865c
Compare
Thank you for the help @gabrielluong 🥇 |
app/src/main/java/org/mozilla/fenix/settings/SyncPreferenceView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/settings/SyncPreferenceView.kt
Outdated
Show resolved
Hide resolved
849865c
to
2031cd4
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.
I think we are super close! I am also super sorry for all these excessive comments that I am leaving in the review. I hope they were generally helpful.
I left some questions just to ask for clarifications.
app/src/main/java/org/mozilla/fenix/settings/SyncPreferenceView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/fenix/settings/SyncPreferenceView.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/mozilla/fenix/settings/logins/SyncPreferenceViewTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/mozilla/fenix/settings/logins/SyncPreferenceViewTest.kt
Show resolved
Hide resolved
app/src/test/java/org/mozilla/fenix/settings/logins/SyncPreferenceViewTest.kt
Show resolved
Hide resolved
app/src/test/java/org/mozilla/fenix/settings/logins/SyncPreferenceViewTest.kt
Outdated
Show resolved
Hide resolved
Don't worry about it! This process increases the overall code quality ❤️ Btw: I'm clicking on "Resolve conversation" once I've addressed/reviewed/answered a comment, as a way to minimize it. That helps me a lot to not get lost in all of them. Feel free to un-resolve them when you add something further on them. |
Sorry, didn't get to this today, will review it tomorrow! |
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.
Let's address the remaining new comments and land this. Please also flag for QA-needede and alert them about testing on both logins and credit cards. Can you also file an issue about that "Reconnect" conversation in #19207 (comment) and flag it for UX-investigation?
Thanks!
For #18262
Pull Request checklist
To download an APK when reviewing a PR:
toggle.credit.cards.logins.sync.mp4