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.

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

add default browser selection overflow menu item #5391

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

LukasPaczos
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/72649045549333/1208944504536342/f

Description

Adds an overflow menu item that advertises setting DuckDuckGo as the default browser.

The introduced item is not yet used anywhere and click listeners/handlers will be added in following PRs.

Steps to test this PR

As an example, apply below diff and the item will show up whenever you open the overflow menu.

diff --git a/app/src/main/java/com/duckduckgo/app/browser/viewstate/BrowserViewState.kt b/app/src/main/java/com/duckduckgo/app/browser/viewstate/BrowserViewState.kt
index bfb487f1e..5db2efe5b 100644
--- a/app/src/main/java/com/duckduckgo/app/browser/viewstate/BrowserViewState.kt
+++ b/app/src/main/java/com/duckduckgo/app/browser/viewstate/BrowserViewState.kt
@@ -34,7 +34,7 @@ data class BrowserViewState(
     val showTabsButton: Boolean = true,
     val fireButton: HighlightableButton = HighlightableButton.Visible(),
     val showMenuButton: HighlightableButton = HighlightableButton.Visible(),
-    val showSelectDefaultBrowserMenuItem: Boolean = false,
+    val showSelectDefaultBrowserMenuItem: Boolean = true,
     val canSharePage: Boolean = false,
     val canSaveSite: Boolean = false,
     val bookmark: SavedSite.Bookmark? = null,

UI changes

Light Dark
Screenshot_20241213_172011 Screenshot_20241213_172029
Screenshot_20241213_172048 Screenshot_20241213_172149

@LukasPaczos LukasPaczos enabled auto-merge (squash) December 13, 2024 16:29
@malmstein malmstein self-assigned this Dec 16, 2024
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-menu-item branch from 8642896 to 541eba3 Compare December 16, 2024 12:07
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-menu-item branch from 541eba3 to a2ec247 Compare December 16, 2024 12:18
@LukasPaczos LukasPaczos disabled auto-merge December 16, 2024 12:39
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.

You don’t need to change the translations, let the service do it for you. Get rid of all the strings changes, and only create new values under donottranslate.xml. Once we are done, we’ll order translations and let the process take its course.

@LukasPaczos
Copy link
Contributor Author

You don’t need to change the translations, let the service do it for you.

I don't want to change translations, I want to reuse the one we already have in the codebase but I'm adjusting the constant's name to better reflect what it does. Is that a problem? Why would we need to run this through the service again if all the information is already there?

If I removed all the strings and added a new one that's not translated, I'd be regressing the functionality that already relies on these translations in other places.

@LukasPaczos
Copy link
Contributor Author

Thanks @malmstein for explaining the string management process!

I reverted a2ec247 for now to avoid conflicts and I'll re-apply it in a separate translation PR.

This is ready for another round.

@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-menu-item branch from 876d680 to b3c64c8 Compare December 18, 2024 17:22
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.

Looks good, thanks @LukasPaczos !

@LukasPaczos LukasPaczos merged commit b7cf1ca into develop Dec 20, 2024
6 checks passed
@LukasPaczos LukasPaczos deleted the feature/lukasz-p/default-browser-menu-item branch December 20, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants