8000 [Notifications Revamp] Add settings banner by sztomek · Pull Request #4032 · Automattic/pocket-casts-android · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Notifications Revamp] Add settings banner #4032

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

Open
wants to merge 11 commits into
base: task/notification-revamp
Choose a base branch
from

Conversation

sztomek
Copy link
Contributor
@sztomek 8000 sztomek commented May 23, 2025

Description

Adding the notification settings banner to the Notification Settings screen when the system notifications permission is not granted.
I have prepared a new component NotificationSettingsBanner following designs: hVHQ8RgGQf1afBWybZExD2-fi-275_5326
I also had to update the existing SettingRow components to support the new 'disabled' state, applying a transparent overlay with 38% alpha.

Testing Instructions

  1. Install app
  2. Don't allow notifications
  3. Navigate to profile -> Settings -> Notifications
  • Turn on notifications banner is displayed
  • All but the 'Settings' section items are disabled and cannot be interacted with
  1. Tap the 'Go to device settings' on the banner
  • System notifications settings screen is displayed
  1. Enable notifications
  2. Navigate back to the app
  • Turn on notifications banner is no longer displayed
  • All settings are now active and can be interacted with

Screenshots or Screencast

Screenshot_20250523_085821

Checklist

  • [ ] If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • [ ] I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@sztomek sztomek added this to the 7.90 milestone May 23, 2025
@sztomek sztomek requested a review from a team as a code owner May 23, 2025 07:19
@sztomek sztomek added the [Type] Feature Adding a new feature. label May 23, 2025
@sztomek sztomek requested review from MiSikora and removed request for a team May 23, 2025 07:19
@wpmobilebot
Copy link
Collaborator
wpmobilebot commented May 23, 2025
📲 You can test the changes from this Pull Request in 📱 Mobile by scanning the QR code below to install the corresponding build.
App Name 📱 Mobile
Build TypedebugProd
Commit2b224a8
Direct Downloadpocketcasts-app-prototype-build-pr4032-2b224a8.apk
📲 You can test the changes from this Pull Request in 🚗 Automotive by scanning the QR code below to install the corresponding build.
App Name 🚗 Automotive
Build TypedebugProd
Commit2b224a8
Direct Downloadpocketcasts-automotive-prototype-build-pr4032-2b224a8.apk
📲 You can test the changes from this Pull Request in ⌚ Wear by scanning the QR code below to install the corresponding build.
App Name ⌚ Wear
Build TypedebugProd
Commit2b224a8
Direct Downloadpocketcasts-wear-prototype-build-pr4032-2b224a8.apk

android:viewportWidth="24"
android:viewportHeight="24">
<path
android:pathData="M11.958,2C12.982,2 13.812,2.895 13.812,4V4.165C15.757,4.538 17.144,5.521 18.008,6.955C18.686,8.081 18.911,9.199 18.911,10V14.241L21.112,18.514C21.455,19.181 21.008,20 20.301,20H14.738C14.738,21.657 13.493,23 11.957,23C10.421,23 9.176,21.657 9.176,20H3.615C2.908,20 2.461,19.181 2.804,18.514L5.005,14.241V10C5.005,9.199 5.23,8.081 5.908,6.955C6.772,5.521 8.159,4.538 10.104,4.165V4C10.104,2.895 10.934,2 11.958,2ZM11.957,21C11.445,21 11.03,20.552 11.03,20H12.884C12.884,20.552 12.469,21 11.957,21ZM18.726,18L17.173,14.986C17.097,14.837 17.057,14.67 17.057,14.5V10C17.057,9.911 17.039,9.707 16.983,9.431C16.888,8.957 16.717,8.483 16.453,8.045C15.687,6.774 14.288,6 11.958,6C9.628,6 8.229,6.774 7.463,8.045C7.199,8.483 7.028,8.957 6.932,9.431C6.877,9.707 6.859,9.911 6.859,10V14.5C6.859,14.67 6.819,14.837 6.743,14.986L5.19,18H18.726Z"

Check warning

Code scanning / Android Lint

Long vector paths Warning

Very long vector path (831 characters), which is bad for performance. Considering reducing precision, removing minor details or rasterizing vector.
Copy link
Contributor
@MiSikora MiSikora 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 and works as expected. The only thing that I see needing a change before merging is the duplicate Banner UI component.

@sztomek sztomek force-pushed the task/notifications-banner branch from e7d13f7 to d2417c4 Compare May 23, 2025 11:48
@sztomek sztomek requested a review from MiSikora May 23, 2025 11:48
@sztomek sztomek force-pushed the task/notifications-add-recommendations branch from baca31f to 8d834e8 Compare May 23, 2025 11:59
Base automatically changed from task/notifications-add-recommendations to task/notification-revamp May 23, 2025 12:17
@sztomek sztomek force-pushed the task/notifications-banner branch from d2417c4 to 2b224a8 Compare May 23, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0