PM-11188 show snackbar after import success. PM-13943 add relay for snackbar events across screen contexts. #4152
Conversation
dseverns-livefront
requested review from
brian-livefront,
david-livefront,
ahaisting-livefront and
a team
as code owners
|
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4152 +/- ##
==========================================
+ Coverage 88.96% 88.98% +0.01%
==========================================
Files 439 441 +2
Lines 38198 38250 +52
Branches 5346 5352 +6
==========================================
+ Hits 33984 34036 +52
Misses 2347 2347
Partials 1867 1867 ☔ View full report in Codecov by Sentry. |
| private val mutableSnackbarRelayMap = | ||
| mutableMapOf<SnackbarRelay, MutableSharedFlow<BitwardenSnackbarData?>>() | ||
|
|
||
| override fun sendSnackbarData(data: BitwardenSnackbarData, relay: SnackbarRelay?) { |
There was a problem hiding this comment.
Does this need to be a nullable SnackbarRelay
There was a problem hiding this comment.
Ahh, now I see why you did that. Not sure how I feel about it
There was a problem hiding this comment.
Instead of the UUIDs, how would you feel about an enum that identified each specific snackbar that requires a relay.
Then you subscribe to all the values you need and you publish to a specific enum value as well.
There was a problem hiding this comment.
I feel like that is the move, may have tried to go a little too agnostic, but no reason why producer and consumer can't know exactly what they are doing when sending/collecting. Great idea!
dseverns-livefront
force-pushed
the
PM-11188-importlogins-success-snackbar
branch
from
7762cbd to
dbd3400
Compare
| */ | ||
| enum class SnackbarRelay { | ||
| VaultSettingsRelay, | ||
| MyVaultRelay, |
There was a problem hiding this comment.
Enums should be capitalized:
VAULT_SETTINGS_RELAY,
MY_VAULT_RELAY,
app/src/main/java/com/x8bit/bitwarden/ui/platform/manager/snackbar/SnackbarRelayManager.kt
Show resolved
Hide resolved
| viewState = ImportLoginsState.ViewState.InitialContent, | ||
| isVaultSyncing = false, | ||
| showBottomSheet = false, | ||
| snackbarRelay = ImportLoginsArgs(savedStateHandle).snackBarRelay, |
There was a problem hiding this comment.
Do we need to pass this in as an argument? Could the screen just know to send the correct enum value?
I believe that is how it worked previously, correct?
There was a problem hiding this comment.
it worked like that cause of the "most recent" relay thing, which had the nullable "relay parameter". I can re-add that, I took the suggestion to move to enums to make it so it was always explicit.
There was a problem hiding this comment.
But with this new system, the previous screen will be listening for the specific enum type and this screen will send the specific enum type. Wouldn't that be the unique identifier we need to make it work?
|
|
||
| private fun getSnackbarDataFlowInternal( | ||
| relay: SnackbarRelay, | ||
| ): MutableSharedFlow<BitwardenSnackbarData?> = |
There was a problem hiding this comment.
Should the BitwardenSnackbarData be nullable?
dseverns-livefront
force-pushed
the
PM-11188-importlogins-success-snackbar
branch
from
98a4bfc to
ed969ee
Compare
…lay to pass snackbars back to previous screens.
dseverns-livefront
force-pushed
the
PM-11188-importlogins-success-snackbar
branch
from
ed969ee to
1895839
Compare
…nackbar events across screen contexts. (#4152)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11188
&
Pt2 of https://bitwarden.atlassian.net/browse/PM-13943
📔 Objective
📸 Screenshots
pm11188.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes