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

824: successful create toast notification #1123

Merged
merged 12 commits into from
Jan 23, 2020
Merged

824: successful create toast notification #1123

merged 12 commits into from
Jan 23, 2020

Conversation

eliserichards
Copy link
Contributor

@eliserichards eliserichards commented Jan 3, 2020 •
edited
Loading

Fixes #824

Testing and Review Notes

  • moved the fun showToastNotification(..) into the RootActivity
  • created ToastNotificationActions to define each toast in a single place

Screenshots or Videos

save_success

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
  • 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 January 3, 2020 23:05
@eliserichards eliserichards requested a review from jhugman January 3, 2020 23:17
jhugman
jhugman previously requested changes Jan 6, 2020
View reviewed changes
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.

We should once we can get the RoutePresenter to find a toast_container in the activity's layout.xml this patch will become much simpler.

HMU if you want to pair on this.

app/src/main/java/mozilla/lockbox/action/RouteAction.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/CreateItemFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/DisplayItemFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/ItemListFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/ItemListFragment.kt Outdated Show resolved Hide resolved
@eliserichards eliserichards dismissed jhugman’s stale review January 7, 2020 23:51

Addressed comments. Ready for re-review!

@eliserichards eliserichards requested a review from jhugman January 7, 2020 23:51
jhugman
jhugman previously requested changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@jhugman jhugman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is looking so much cleaner.

I especially like the use of decorView.

There are still a few changes to be done, but this is really becoming quite a neat bit of work. Well done.

app/src/main/java/mozilla/lockbox/presenter/ItemListPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/ItemMutationPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/ItemMutationPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/RoutePresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/CreateItemPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/action/ToastNotificationAction.kt Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/action/ToastNotificationAction.kt Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/model/ToastNotificationViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/DisplayItemPresenter.kt Show resolved Hide resolved
@jhugman
Copy link
Contributor

jhugman commented Jan 8, 2020

Also: you should also add ToastNotificationAction to the list of actions that need to be trimmed in the RouteStore. The block is linked here.

jhugman
jhugman reviewed Jan 22, 2020
View reviewed changes
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.

Fresh eyes drive by.

app/src/main/java/mozilla/lockbox/presenter/ItemListPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/presenter/DisplayItemPresenter.kt Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/model/ToastNotificationViewModel.kt Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
@eliserichards eliserichards dismissed jhugman’s stale review January 23, 2020 13:47

Addressed comments :)

jhugman
jhugman previously requested changes Jan 23, 2020
View reviewed changes
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. Let's pair for the last bit.

app/src/main/java/mozilla/lockbox/model/ToastNotificationViewModel.kt Outdated
data class ToastNotificationViewModel(
@StringRes val strId: Int,
@DrawableRes val img: Int,
val deletedItem: String? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this a string to be inserted into the strId string. We can't say it's a deletedItem.

app/src/main/java/mozilla/lockbox/presenter/EditItemPresenter.kt Outdated
} else {
null
listOf(ItemDetailAction.EndEditItemSession)
Copy link
Contributor

Choose a reason for hiding this comment

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

Set this to null please.

app/src/main/java/mozilla/lockbox/presenter/RoutePresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/mozilla/lockbox/view/RootActivity.kt Outdated Show resolved Hide resolved
@eliserichards eliserichards dismissed jhugman’s stale review January 23, 2020 16:15

Paired and addressed changes :D

jhugman
jhugman approved these changes Jan 23, 2020
View reviewed changes
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.

Lovely! Please land celebrate test and demo toute suite.

@eliserichards eliserichards merged commit fa4e6d9 into master Jan 23, 2020
@eliserichards eliserichards deleted the 824-new-entry-toast branch February 11, 2020 15:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jhugman jhugman jhugman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display toast when new entry is saved
2 participants
@eliserichards @jhugman