Add notifyme to waitlist #2843
Add notifyme to waitlist #2843
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@+id/statusTitle" /> | ||
|
|
||
| <FrameLayout |
There was a problem hiding this comment.
This screen is used in 3 different states. Depending on the state, some elements are visible/hidden.
Notifyme component should only appear when user has joined the waitlist (1 of those 3 states), but it handles visibility internally. To avoid that, I'm wrapping the view with a framelayout, not ideal but works for this simple scenario (and we will remove the screen in the future).
There was a problem hiding this comment.
Have you tested this in all scenarios? The component uses Lifecycle to check for permissions, and it hooks into it onAttach. I'm worried that you might be missing hooks if the FL is one by default.
There was a problem hiding this comment.
Moving answer to Asana. I tested it and works fine, but I want to double-check check I'm not missing a scenario I don't know.
malmstein
left a comment
malmstein
left a comment
There was a problem hiding this comment.
Nit about the lifecycle of the NotifyMe view. If you have tested that the component can react to enable / disabling notification permissions from the phone Settings, then there's nothing to worry about. Approving this since it's the desired process, but hoping you have tested that scenario.
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@+id/statusTitle" /> | ||
|
|
||
| <FrameLayout |
There was a problem hiding this comment.
Have you tested this in all scenarios? The component uses Lifecycle to check for permissions, and it hooks into it onAttach. I'm worried that you might be missing hooks if the FL is one by default.
cmonfortep
force-pushed
the
feature/cristian/windows_waitlist
branch
from
0dfc124 to
2b866fe
Compare
cmonfortep
force-pushed
the
feature/cristian/windows_waitlist_notifyme
branch
3 times, most recently
from
9fdcb05 to
0da06e7
Compare
| binding.statusTitle.text = getString(R.string.windows_waitlist_on_the_list_title) | ||
| binding.waitlistDescription.text = getText(R.string.windows_waitlist_on_the_list_notification) | ||
| binding.waitlistDescription.text = getJoinedQueueDescriptionText(binding.waitlistNotifyMe.isVisible) | ||
| binding.waitlistNotifyMe.setOnVisibilityChange( |
There was a problem hiding this comment.
@malmstein as I mentioned, we change the description copy based on the notifyme component visiblity.
| binding.waitlistDescription.text = getText(R.string.windows_waitlist_description) | ||
| } | ||
|
|
||
| private fun getJoinedQueueDescriptionText(notifyMeVisible: Boolean): String { |
There was a problem hiding this comment.
This is new as well
cmonfortep
force-pushed
the
feature/cristian/windows_waitlist_notifyme
branch
from
0da06e7 to
80501b7
Compare
<!--
Note: This checklist is a reminder of our shared engineering
expectations.
The items in Bold are required
If your PR involves UI changes:
1. Upload screenshots or screencasts that illustrate the changes before
/ after
2. Add them under the UI changes section (feel free to add more columns
if needed)
3. Make sure these changes are tested in API 23 and API 26
If your PR does not involve UI changes, you can remove the **UI
changes** section
-->
Task/Issue URL:
https://app.asana.com/0/488551667048375/1203935342025551/f
### Description
Adds feature flag onto waitlist feature.
There are 2 entry points:
- From macOS screen, if feature disabled, do not show link to waitlist
- From settings screen, if feature disabled, do not show setting item
### Steps to test this PR
_Feature 1_
- [x] In PrivacyFeatureName, replace PRIVACY_REMOTE_CONFIG_URL with
`https://jsonblob.com/api/jsonBlob/1075345853253959680`
- [x] Fresh install
- [x] Go to settings, ensure no entry point to wd waitlist present
- [x] Go to macos settings, ensure no entry point to wd waitlist present
_Feature 2_
- [x] Fresh install
- [x] Go to settings, ensure entry point to wd waitlist is present
- [x] Go to macos settings, ensure entry point to wd waitlist present
### UI changes
| Before | After |
| ------ | ----- |
!(Upload before screenshot)|(Upload after screenshot)|
2 participants
Task/Issue URL: https://app.asana.com/0/488551667048375/1203975081655927/f
Description
Adds notify me component to waitlist
Steps to test this PR
Feature 1
UI changes