Would you mind explaining why we need this? I believe this is done so that the banner reappears when the state changes in the ViewModel. Is that it?

I'm worried though, that we now have UI state in two places and we should avoid that.

Would it be possible to keep the state fully in ViewModel (including the dismissed state)?

Thanks for pointing this out. 🙇 I agree that the approach wasn't correct. I've refactored it.

Looks better now! I'm worried about the current logic though. Considering a scenario like this:

1. User deletes the currently selected Avatar
2. Alert banner appears
3. User dismisses the banner
4. User tries to delete another avatar, but the request fails
5. Alert banner reappears -- this shouldn't happen
6. User dismisses the banner
7. User tries to delete another avatar again, this time successfully
8. Alert banner reappears -- this shouldn't happen

Here's the recording showing alert banner reappearing:

I do believe we should respect the user's action to dismiss the banner for longer. I would propose to reset that user action only if the user went from avatar selected to no avatar state. Here's an updated scenario where the alert banner would reappear:

1. User deletes the currently selected Avatar
2. Alert banner appears
3. User dismisses the banner
4. User tries to delete another avatar, but the request fails
5. Alert banner DOESN'T reappear
6. User tries to delete another avatar again, this time successfully
7. Alert banner DOESN'T reappear
8. User selects an avatar
9. User deletes the currently selected Avatar
10. Alert banner appears - Only here

WDYT?

It makes sense. I've refactored that and added DeleteAvatarAlertStatus to make it clearer the current status of the AlertBanner.

The behavior looks good now!

💡 💡 💡 💡

I do feel like we could simply the implementation and make it more future proof though.

The Alert banner visibility is defined by two values isAvatarSelected (emailAvatars.selectedAvatarId == null) and isBannerDismissed.
The visibility condition would look like this: banner.visible = !isAvatarSelected && !isBannerDismissed

The first value isAvatarSelected is already in the UI state so technically we don't need to do anything extra here.
The second one isBannerDissmed should change based on the behavior in two places:

1. When the users dismisses it manually it should be set to true
2. When the isAvatarSelected changes from true to false it should be reset to false

Point 1 is already handled in your implementation. Point 2 is to, kinda, but manually in each potential place where the emailAvatars.selectedAvatarId value could change. This is where I think we might end up with some bugs in the future as we could simply forget to update it in new places.

As I mentioned above we already have the emailAvatars.selectedAvatarId in the UI state so we could simply observe it and react when the value changes. Here's an example code:

_uiState
            .filter { it.emailAvatars != null } //We don't care about this until we get this value
            .map { it.emailAvatars?.selectedAvatarId != null }
            .distinctUntilChanged()
            .onEach { isAvatarSelected ->
                if (isAvatarSelected) {
                    _uiState.update { currentState ->
                        currentState.copy(
                            nonSelectedAvatarAlertVisible = DeleteAvatarAlertStatus.HIDDEN,
                        )
                    }
                } else {
                    _uiState.update { currentState ->
                        currentState.copy(
                            nonSelectedAvatarAlertVisible = DeleteAvatarAlertStatus.VISIBLE,
                        )
                    }
                }
            }
            .launchIn(viewModelScope)

I've used DeleteAvatarAlertStatus from your implementation but not sure we need it.

With the above code, we can get rid of the shouldShowNonSelectedAvatarAlert() function completely and the isBannerDismissed should be properly reset whenever the emailAvatars.selectedAvatarId value changes no matter what triggered it.

Sorry, for not catching this earlier, but I was focused on the behavior rather than the code! Let me know what you think!

I was looking for a similar approach, but I couldn't come up with anything. Looks like I could have been more inspired.

I've updated the code, removing the DeleteAvatarAlertStatus, which is totally unnecessary right now, and observing the flow to update the nonSelectedAvatarAlertVisible, as you suggested.

Thanks for your review, @AdamGrzybkowski, I really appreciate it. I hope it looks better now.

I wonder if we could customize this animation. Currently when this banner gets hidden the whole UI is moved up, rather than the top of the bottom sheet just going down. Feels unnatural.

I've been playing with the AnimatedVisibility params and options. It's not easy to achieve, or at least it's not pretty straightforward.

I agree it's nice to have, but I don't want to dedicate too much time to this now. What do you think about creating a ticket and returning to it when we have more "free time"?