-
-
Notifications
You must be signed in to change notification settings - Fork 846
Add notification permission bottom sheet #6094
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
base: feature/use_new_onboarding
Are you sure you want to change the base?
Add notification permission bottom sheet #6094
Conversation
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Fixed
Show fixed
Hide fixed
jpelgrom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy flow functionally works OK, even though I personally preferred the placement in the onboarding flow.
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewPresenter.kt
Outdated
Show resolved
Hide resolved
...Test/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreenScreenshotTest.kt
Outdated
Show resolved
Hide resolved
...oid/webview/WebViewContentScreenScreenshotTest/WebView request notification permission_0.png
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a notification permission bottom sheet that appears in the WebView screen after the dashboard loads, replacing the previous dedicated permission screen in the onboarding flow. The implementation uses the Home Assistant design system components and stores the user's permission preference to avoid repeated prompts.
Key Changes
- Added preference storage for tracking whether to ask for notification permission
- Implemented a modal bottom sheet UI component requesting notification permission
- Integrated the permission request into the WebView screen after initialization
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
common/src/main/res/values/strings.xml |
Added localized strings for notification permission dialog title, content, and button labels |
common/src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepository.kt |
Added interface methods for managing notification permission preference |
common/src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepositoryImpl.kt |
Implemented preference storage methods with default value of true |
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewPresenter.kt |
Added presenter interface methods to expose permission preference logic |
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewPresenterImpl.kt |
Implemented presenter methods delegating to PrefsRepository |
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt |
Added NotificationPermission composable with bottom sheet UI and integrated into main screen |
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt |
Added state management and callback handling for permission request flow |
app/src/screenshotTest/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreenScreenshotTest.kt |
Updated screenshot tests with new parameters and added test case for permission request |
...Test/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreenScreenshotTest.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepositoryImpl.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt
Outdated
Show resolved
Hide resolved
common/src/main/kotlin/io/homeassistant/companion/android/common/data/prefs/PrefsRepository.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Fixed
Show fixed
Hide fixed
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreen.kt
Outdated
Show resolved
Hide resolved
...Test/kotlin/io/homeassistant/companion/android/webview/WebViewContentScreenScreenshotTest.kt
Outdated
Show resolved
Hide resolved
jpelgrom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and function works as expected. I'm interested in the background behind design but that's not a blocker for merging.
Co-authored-by: Joris Pelgröm <[email protected]>
8598bea to
92093be
Compare
|
|
||
| override suspend fun onNotificationPermissionResult(granted: Boolean) { | ||
| if (granted && BuildConfig.FLAVOR != "full") { | ||
| settingsDao.insert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setting should be added for each new server though when using minimal, while this pop-up is only shown once if the permission is missing (so probably the first server added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's where having it in the onboarding was easier :/. Should we in minimal always show the sheet ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would always show it for minimal, because it's setting up a persistent connection which may not be necessary for each server but drain some battery.
Summary
In the new onboarding the notification permission is displayed when we reach the dashboard instead of having a dedicated screen.
Checklist
Screenshots
Any other notes
This PR is based on #6081