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

Merged
merged 8 commits into from
Dec 2, 2024
Merged

Update Settings design: Add new SettingsListItem #5318

merged 8 commits into from
Dec 2, 2024

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 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

Copy link
Contributor Author

@mikescamell mikescamell Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Won't using DaxListItem enforce using Views I actually do not need? i.e. secondaryText, trailingSwitch

Also the spacing for the trailingIcon was not suitable for the StatusIndicator and I did not want to mess around with that too much considering how widely it's used.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot 👍

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the colour of the indicator does not change based on light/dark mode from checking iOS

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the StatusIndicator will be used on sub-screens as well. It's developed further for the sub-screens in another PR

@malmstein malmstein self-assigned this Nov 29, 2024
@mikescamell
duplicating content settings and privacy
c6be3dd
@mikescamell
add new icons
5d543b0
@mikescamell
add new SettingsListItem
a0b9993
@mikescamell
switch to SettingsListItem in privacy section
2962eb3 
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
7c23cf1 
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
058fccc 
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
6aa74a2
@mikescamell
Fix status indicator alignment
26b24c0
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-new-setting-list-item branch from ea26e05 to 26b24c0 Compare December 2, 2024 09:27
malmstein
malmstein approved these changes Dec 2, 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.

Thanks for addressing the comments, good work!

@mikescamell mikescamell merged commit 010d9cf into develop Dec 2, 2024
7 checks passed
@mikescamell mikescamell deleted the feature/mike/update-settings/add-new-setting-list-item branch December 2, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malmstein malmstein malmstein approved these changes

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

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