Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update Settings design: Add new SettingsListItem #5318

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Update Settings design: Add new SettingsListItem #5318

wants to merge 7 commits into from

Conversation

mikescamell
Copy link
Contributor

@mikescamell mikescamell commented Nov 26, 2024 •
edited by malmstein
Loading

Task/Issue URL: https://app.asana.com/0/1207908166761516/1208785622935231/f

Description

  • Adds all new Icons for Settings (not all are implemented)
  • Adds new SettingsListItem
    • Also added to the ADS screen
    • Designs
  • Tackles the privacy section without any copy changes which will be done in a later PR

Steps to test this PR

Pre-requisites:

  • newSettings feature flag must be enabled
  • Easier to start with a fresh launch
  • Don't enable DDG as default browser

Default Browser On/Off

  • Open New Settings screen
  • Default browser should be off ⚪
  • Press default browser
  • Select DDG
  • Press back to get back to the New Settings screen
  • Default browser should be on 🟢
  • Press default browser
  • Select another browser
  • Press back to get back to the New Settings screen
  • Default browser should be off ⚪

Private Search

  • Should be on and is always on

Web Tracking Protection

  • Should be on and is always on

Cookie Pop-Up Protection

  • Open New Settings screen
  • Cookie Pop-Up Protection should be on 🟢
  • Press Cookie Pop-Up Protection
  • Toggle off "Let DuckDuckGo handle cookie pop-ups"
  • Press back
  • Cookie Pop-Up Protection should be off ⚪
  • Press Cookie Pop-Up Protection
  • Toggle on "Let DuckDuckGo handle cookie pop-ups"
  • Press back
  • Cookie Pop-Up Protection should be on 🟢

App Tracking Protection

  • Open New Settings screen
  • App Tracking Protection should be off ⚪
  • Press App Tracking Protection
  • Toggle on App Tracking Protection
  • Press back
  • App Tracking Protection should be on 🟢
  • Press App Tracking Protection
  • Toggle off App Tracking Protection
  • Press back
  • App Tracking Protection should be off ⚪

Email Protection

  • Open New Settings screen
  • Email Protection should be off ⚪
  • Press Email Protection
  • Follow steps to turn on Email Protection
  • Open New Settings screen
  • Email Protection should be on 🟢
  • Press Email Protection
  • Sign out
  • Open New Settings screen
  • Email Protection should be off ⚪

UI changes

Before After
Screenshot_20241126_145126 Screenshot_20241126_145056
Screenshot_20241126_145134 Screenshot_20241126_145109
Screenshot_20241126_145502 Screenshot_20241126_145543

Demo

Screen_recording_20241126_145018.mp4

@mikescamell Graphite App
Copy link
Contributor Author

mikescamell commented Nov 26, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@mikescamell mikescamell mentioned this pull request Nov 26, 2024
Merged
32 tasks
@mikescamell mikescamell marked this pull request as ready for review November 26, 2024 11:23
@mikescamell mikescamell marked this pull request as draft November 26, 2024 11:26
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-new-setting-list-item branch 2 times, most recently from 8d2f2e6 to abf33e2 Compare November 26, 2024 14:30
@mikescamell mikescamell marked this pull request as ready for review November 26, 2024 18:13
Base automatically changed from feature/mike/update-settings/add-feature-flag to develop November 27, 2024 09:47
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-new-setting-list-item branch from abf33e2 to 38b133a Compare November 27, 2024 16:18
@mikescamell
Copy link
Contributor Author

Android CI Test failing (not related to this work) but a fix has been raised: #5316

@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-new-setting-list-item branch from 38b133a to 0782d10 Compare November 28, 2024 09:50
@mikescamell mikescamell mentioned this pull request Nov 28, 2024
Open
36 tasks
@mikescamell
duplicating content settings and privacy
68420c6
@mikescamell
add new icons
6b40e08
@mikescamell
add new SettingsListItem
797440d
@mikescamell
switch to SettingsListItem in privacy section
7799cb0 
Also adding all the new icons, removing secondary text and itemState, and adding the new isOn for search and web tracking settings

Next commit we'll ensure the indicators are on/off
@mikescamell
set items to on/off based on state
fac2d06 
I've removed appTrackingProtectionOnboardingShown state as only have 2 states now, on/off, but I will follow up and ask Thomas if this is expected
@mikescamell
add SettingsListItem to ADS
f475c87 
I think this is worth having on the ADS screen, even if not officially a component. Happy to move it if needs be.
@mikescamell
formatting
d8f02fd
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-new-setting-list-item branch from 0782d10 to d8f02fd Compare November 29, 2024 12:28
@mikescamell mikescamell mentioned this pull request Nov 29, 2024
Open
57 tasks
malmstein
malmstein requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

When several status indicators are displayed vertically, you can see how they go out of alignment if the text (On/Off) is different. The issue is much more visible if you use another languages.

common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/SettingsListItem.kt
context: Context,
attrs: AttributeSet? = null,
defStyleAttr: Int = R.attr.oneLineListItemStyle,
) : ConstraintLayout(context, attrs, defStyleAttr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should implement DaxListItem. Look at BookmarksListItem for reference

common/common-ui/src/main/res/layout/view_status_indicator.xml
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="center_vertical"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. The way you’ve built it only aligns if all the texts are the same. If you looks closely, even with On - Off as texts there’s a difference.

Screenshot 2024-11-29 at 21 07 26

The more text present, the wider the View should be, but only growing from the text side.

common/common-ui/src/main/res/color/status_indicator_color_selector.xml
~ limitations under the License.
-->

<selector xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools">
Copy link
Contributor

Choose a reason for hiding this comment

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

are the colors the same in light and dark? seems odd

common/common-ui/src/main/java/com/duckduckgo/common/ui/view/listitem/SettingsListItem.kt
}

fun setStatus(isOn: Boolean) {
statusIndicator.setStatus(isOn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a different View just for the status indicator? I think you can have it all in here. The status indicator is part of the Item, and won’t be used separately.

@malmstein malmstein self-assigned this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malmstein malmstein malmstein requested changes

@nalcalag nalcalag Awaiting requested review from nalcalag nalcalag is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@malmstein malmstein

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikescamell @malmstein