Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

1152: Toast placement for copy, create, delete #1166

Merged
merged 4 commits into from
Feb 11, 2020

Conversation

eliserichards
Copy link
Contributor

@eliserichards eliserichards commented Feb 3, 2020

Fixes #1152

Specify each bottom margin for the toast notifications. The bottomMargin value set based on the nav location: toasts on the item list will appear above the + create button, and other toasts will have the normal bottom margin at the bottom of the screen.

Testing and Review Notes

  • copy username/copy password appear at the bottom of the screen
  • delete confirmation appears above the + button on the item list
  • create confirmation appears on the item detail screen at the bottom (same place as copy)

Screenshots or Videos

(save gif tbd on #1163)

copy_toasts

delete_toast

To Do

  • double check the original issue to confirm it is fully satisfied
  • add testing notes and screenshots in PR description to help guide reviewers
  • add unit tests
    • optional: consider adding instrumentation (integration/UI) tests
  • consider running this branch in a debug simulator and check for memory leak notifications or any warnings
  • request the "UX" team perform a design review (if/when applicable)
  • make sure CI builds are passing (e.g.: fix lint and other errors)
  • check on the accessibility of any added UI

@eliserichards eliserichards requested a review from a team as a code owner February 3, 2020 13:59
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm conflicted about this. If we land this patch as is, we're incurring some avoidable technical debt.

However, I'm not sure that toast notification is going to be used more than we have it, and I don't think that this is going to cause enough friction to development as to have something done about it.

I have left two paths forward in the comment; decide if and which you want to do, and then let's conflab.

@eliserichards eliserichards dismissed jhugman’s stale review February 10, 2020 22:37

Addressed comments, the easy way ;)

Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. I won't make you do another review cycle, but I'd like you to make the changes as suggested.

@@ -122,7 +122,18 @@ abstract class RoutePresenter(
val layoutInflater = LayoutInflater.from(activity)

toast.view = layoutInflater.inflate(R.layout.toast_view, container, false)
toast.setGravity(Gravity.FILL_HORIZONTAL or Gravity.BOTTOM, 0, 216)

val bottomMargin = if (action is ToastNotificationAction.ShowDeleteToast) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy: choose the bottom margin based on whether or not the current destination is the ItemList.

val bottomMargin = if (navController.currentDestination?.id == R.id.fragment_item_list) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we can make all toasts that are displayed on the Item List avoid the + button, not just the delete one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that makes perfect sense. Done.

toast.setGravity(
Gravity.FILL_HORIZONTAL or Gravity.BOTTOM,
0,
activity.resources.getDimension(bottomMargin).toInt()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good.

@eliserichards eliserichards merged commit 536b0de into master Feb 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The deletion confirmation snackbar is displayed over the + button
2 participants