Skip to content

Conversation

@wzieba
Copy link
Contributor

@wzieba wzieba commented May 5, 2023

Closes: #8936

Description

This PR targets the feature branch.

It introduces a redesign of the Privacy Screen.

Testing instructions

Smoke test if switches work and save state after closing the screen/app.

Images/gif

Android iOS
image

I had to translate iOS design to Material Design. I was inspired by this widget in Material 3 specification:
image

I believe it works well in our context 👍

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@wzieba wzieba added this to the 13.6 milestone May 5, 2023
@wzieba wzieba added type: task An internally driven task. feature: privacy Related to the privacy choice projects labels May 5, 2023
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 5, 2023

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@codecov-commenter
Copy link

codecov-commenter commented May 8, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (74c131c) 43.12% compared to head (b92f37f) 43.11%.

❗ Current head b92f37f differs from pull request most recent head f8d57b7. Consider uploading reports for the commit f8d57b7 to get more accurate results

Additional details and impacted files
@@                       Coverage Diff                        @@
##             feature/new_privacy_screen    #8968      +/-   ##
================================================================
- Coverage                         43.12%   43.11%   -0.01%     
  Complexity                         3950     3950              
================================================================
  Files                               823      823              
  Lines                             43370    43380      +10     
  Branches                           5662     5664       +2     
================================================================
  Hits                              18704    18704              
- Misses                            23017    23027      +10     
  Partials                           1649     1649              
Impacted Files Coverage Δ
...merce/android/ui/prefs/PrivacySettingsViewModel.kt 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@wzieba wzieba linked an issue May 8, 2023 that may be closed by this pull request
1 task
@wzieba wzieba marked this pull request as ready for review May 8, 2023 18:24
@wzieba wzieba requested review from a team and atorresveiga and removed request for a team May 8, 2023 18:24
@atorresveiga atorresveiga self-assigned this May 8, 2023
import com.woocommerce.android.ui.compose.theme.WooThemeWithBackground

@Composable
fun PrivacySettingsScreen(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

state: PrivacySettingsViewModel.State,
onAnalyticsSettingChanged: (Boolean) -> Unit,
onReportCrashesChanged: (Boolean) -> Unit,
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np: WDYT if we pass a modifier parameter to all our composables[PrivacySettingsScreen, OptionRow] (as stated in the best practices)

@atorresveiga
Copy link
Contributor

I had to translate iOS design to Material Design. I was inspired by this widget in Material 3 specification

Nice! I loved the result 👏

@atorresveiga
Copy link
Contributor

I believe we should increase the control contrast slightly when in the unchecked state. It is hard to see the control, especially in dark mode.

Here is a comparison with a switch control from the settings screen

Settings Screen Privacy Settings
Screenshot 2023-05-09 at 11 28 14 Screenshot 2023-05-09 at 11 28 36

@atorresveiga
Copy link
Contributor

IMO we should follow the same text sizes, spacing and colors we have in the Settings screen

  • Section title is the same size as setting title
  • Setting description has a bigger font and a different text color
  • The space between Setting title and description is bigger
Settings Screen Privacy Settings
Screenshot 2023-05-09 at 11 43 58 Screenshot 2023-05-09 at 11 44 15

@atorresveiga
Copy link
Contributor

np: I'm not in love with the islands that get formed with the descriptions over the gray background and the controls on a different elevation. IMO it looks nice on the Settings screen, where all the info is at the same elevation.
Feel free to ignore this comment.

Settings screen Privacy settings
Screenshot 2023-05-09 at 11 55 24 Screenshot 2023-05-09 at 11 55 24

@atorresveiga
Copy link
Contributor

The code looks great, @wzieba, and is working as expected. Kudos for refactoring this screen and using Jetpack Compose. The first review is done, I've left some suggestions, let me know WDYT 😄

@peril-woocommerce
Copy link

peril-woocommerce bot commented May 9, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@wzieba
Copy link
Contributor Author

wzieba commented May 10, 2023

Thanks, @atorresveiga for the review!

I agree with your comments. I've tried to iterate over the design to address them and make it similar to the rest of the privacy settings. It's not ideal and requires some debatable ideas like fun textAppearanceWooBody2() but I think it's better now.

Because there are a few PRs in the pipeline, I'll go and merge this PR. But this targets the feature branch, so please feel free to comment here, and I'll address your comments in another PR.

Thanks again!

Before Now
image image

@wzieba wzieba removed this from the 13.6 milestone May 10, 2023
@wzieba wzieba merged commit 79e5808 into feature/new_privacy_screen May 10, 2023
@wzieba wzieba deleted the issue/8936_privacy_screen_redesign branch May 10, 2023 10:25
@wzieba wzieba mentioned this pull request May 10, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: privacy Related to the privacy choice projects type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Privacy Choices] Privacy Screen Redesign

5 participants