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 feature flag & new Settings screen #5300

Merged
merged 14 commits into from
Nov 27, 2024
Merged

Update Settings design: Add feature flag & new Settings screen #5300

merged 14 commits into from
Nov 27, 2024

Conversation

mikescamell
Copy link
Contributor

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

Task/Issue URL: https://app.asana.com/0/488551667048375/1208785611228225/f

Description

Adds a feature flag and new screen for the new updated Settings screen designs.

For now it's the same layout as before, this is just laying the groundwork for the rest of the implementation.

So you can see that the flag is working, the new Settings screen has a temporary title of "New Settings" which will be removed when the UI changes enough that it's perceivable.

I also added the ability to trigger notifications from dev settings so it's easier to test the ClearData notification route to opening settings.

Steps to test this PR

  • Turn on the feature flag "newSettings"

Opening Settings from the Browser menu

  • Click overflow menu
  • Press Settings button
  • Ensure "New Settings" is displayed at the top

Opening Settings from ADS shortcut

  • Add the ADS shortcut via phone homescreen
  • Press Shortcut
  • ADS screen should open
  • Press back
  • Ensure "New Settings" is displayed at the top

Clear Data Notification

  • Open new Notifications screen in Dev Settings
  • Click "Data clearing set to manual"
  • Click on the notification
  • Ensure "New Settings" is displayed at the top

VPN Disabled Notification

  • Open new Notifications screen in Dev Settings
  • Click "NetP Disabled"
  • Click on the notification
  • Ensure "New Settings" is displayed at the top

Clear Data Notification

  • Open new Notifications screen in Dev Settings
  • Click "Data clearing set to manual"
  • Click on the notification
  • Ensure "New Settings" is displayed at the top

New Tab Page

  • Ensure "pluginNewTabPage" feature flag is enabled
  • Open a new tab
  • Click the "Settings" shortcut
  • Ensure "New Settings" is displayed at the top

Tab Switcher

  • Open Tab Switcher screen
  • Click overflow menu
  • Click "Settings"
  • Ensure "New Settings" is displayed at the top

Privacy Pro

  • Simplest way to see it is to edit ln 126 in SpecialUrlDetector to true and load a web page
  • Press back
  • New settings should be visible

UI changes

N/A

@mikescamell Graphite App
Copy link
Contributor Author

mikescamell commented Nov 21, 2024 •
edited
Loading

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

@mikescamell mikescamell marked this pull request as ready for review November 21, 2024 15:24
@malmstein malmstein self-assigned this Nov 22, 2024
malmstein
malmstein requested changes Nov 23, 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.

I don’t understand why have you copied the screen. What’s the benefit of doing it? We should start the new screen from scratch.

There’s a feature flag, have it disabled by default also for internal users. When you want to try the new screen, manually flip the switch and test it.

malmstein
malmstein reviewed Nov 23, 2024
View reviewed changes
app/src/main/java/com/duckduckgo/app/settings/NewSettingsViewModel.kt

@SuppressLint("NoLifecycleObserver")
@ContributesViewModel(ActivityScope::class)
class NewSettingsViewModel @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, this is inheriting all the bad practices we’ve been adding over the years. It’s going to take you much longer than if you start the screen from scratch with the new design.

...ions/subscriptions-impl/src/main/java/com/duckduckgo/subscriptions/impl/RealSubscriptions.kt Outdated Show resolved Hide resolved
@mikescamell
Copy link
Contributor Author

I don’t understand why have you copied the screen. What’s the benefit of doing it? We should start the new screen from scratch.

There’s a feature flag, have it disabled by default also for internal users. When you want to try the new screen, manually flip the switch and test it.

The idea is get the UI work done first, using the existing screen as the basis and then start working on the plugins, and at that point look to update the screen. I believe this de-risks the project if the plugins work takes longer or I hit a snag. You could argue we could ship this as soon as the UI work is done and then ship the plugins work later.

The feature flag for this work is already disabled by default.

@mikescamell
duplicate settings screen
0e874e3 
Note we've also duplicated the xml layout and are now referring to it's binding in the Activity

I used a new title of "New Settings" for the activity so it's easy to differentiate and notice if you're using the flagged version. I will remove this once we have the new UI for the items and it's more obvious it's the updated version
@mikescamell
add feature toggle
27ba691
@mikescamell
add notifications to dev settings
45b4db0 
NotificationsActivity in the next commit
@mikescamell
add notifications screen and scheduled notifications
a814203 
You can see in the viewmodel that I had some issues with the SurveyAvailableNotification

1. The SurveyAvailableNotification needs a notification in the database, otherwise we get a NoElement exception as we call "last()" on the list of surveys when getting them in the intent
2. The intent hits a repo that hits a DAO, that is not a suspend function, so we have to be off the main thread

Next we'll add VPN notifications
@mikescamell
add disabled vpn notification
4747a7f
@mikescamell
formatting
61f3c48
@mikescamell
revert formatting change
3c21657
@mikescamell
Rename SettingsActivity to LegacySettingsActivity
c23f5d1
@mikescamell
fix NewSettingsActivity using wrong ViewModel
59e7699
@mikescamell
Create SettingsActivity
8e46319 
This will be our bridge to either the Legacy or NewSettingsActivity
@mikescamell
kill navigation via GlobalActivityStarter
e9c68ed 
No longer needed
@mikescamell
add feature toggle to settings-api
1f4714c
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-feature-flag branch from b89ca53 to ef451b7 Compare November 25, 2024 17:22
@mikescamell mikescamell force-pushed the feature/mike/update-settings/add-feature-flag branch from ef451b7 to 1a810d9 Compare November 25, 2024 17:25
@mikescamell mikescamell mentioned this pull request Nov 26, 2024
Merged
42 tasks
malmstein
malmstein requested changes Nov 26, 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.

The toggle should be disabled by default

settings/settings-api/src/main/java/com/duckduckgo/settings/api/NewSettingsFeature.kt Outdated Show resolved Hide resolved
malmstein
malmstein approved these changes Nov 27, 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.

works as expected, thanks @mikescamell !

@mikescamell mikescamell merged commit fdd1cfa into develop Nov 27, 2024
7 checks passed
@mikescamell mikescamell deleted the feature/mike/update-settings/add-feature-flag branch November 27, 2024 09:47
This was referenced Nov 28, 2024
Open
Open
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

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