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

No saved entries (list view)#397

Merged
devinreams merged 2 commits intomasterfrom
21-no-saved-entries
Jan 23, 2019
Merged

No saved entries (list view)#397
devinreams merged 2 commits intomasterfrom
21-no-saved-entries

Conversation

@ioana-farcas
Copy link
Copy Markdown
Contributor

@ioana-farcas ioana-farcas commented Jan 22, 2019

Fixes #21

Testing and Review Notes

Note: Pull to refresh should work if there are no entries in the list.

Screenshots or Videos

no_entries

To Do

  • add “WIP” to the PR title if pushing up but not complete nor ready for review
  • 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

@ioana-farcas ioana-farcas requested a review from a team as a code owner January 22, 2019 18:24
@ghost ghost assigned ioana-farcas Jan 22, 2019
@ghost ghost added the in progress label Jan 22, 2019
@ioana-farcas ioana-farcas requested a review from a team January 22, 2019 18:26
Copy link
Copy Markdown
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

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

This looks really nice!

The requested change is to remove uncommented code. Otherwise I have nits that are ok to land as-is.

Comment thread app/src/test/java/mozilla/lockbox/adapter/ItemListAdapterTest.kt Outdated
Comment thread app/src/main/res/values/strings.xml Outdated
Comment thread app/src/main/res/drawable/ic_placeholder_no_entries.xml
Copy link
Copy Markdown
Contributor

@nickbrandt nickbrandt left a comment

Choose a reason for hiding this comment

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

Perfect, nice work! 👍

screen shot 2019-01-22 at 1 51 39 pm

@ioana-farcas ioana-farcas force-pushed the 21-no-saved-entries branch 2 times, most recently from 0b18da7 to 8ac670e Compare January 23, 2019 10:54
Copy link
Copy Markdown
Contributor

@devinreams devinreams left a comment

Choose a reason for hiding this comment

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

Looks good here on Pixel 2 👍

Though there was a long (I connected to a VPN through AUS so accidentally testing a very slow connection) wait with the fully blank screen (no "placeholder" entries in background) and no visual indicator the app is syncing.

I might advocate for looking at this bug soon since I think the state/code/behavior may all be related: #388

@devinreams
Copy link
Copy Markdown
Contributor

Based on @linuxwolf's code review and @nickbrandt's approval I believe this is good to merge as-is (all three items addressed).

But, we can leave it open for another hour or two in case anyone (else) in @mozilla-lockbox/mobile-engineering would like to take (another) look after our standup.

Thanks for your help on this feature @ioana-farcas 🙇

@devinreams devinreams dismissed linuxwolf’s stale review January 23, 2019 14:58

Code review items have been addressed

@devinreams devinreams requested a review from a team January 23, 2019 14:59
Copy link
Copy Markdown
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

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

r+

:shipit:

@devinreams devinreams merged commit dd9c986 into master Jan 23, 2019
@ghost ghost removed the in progress label Jan 23, 2019
@devinreams devinreams deleted the 21-no-saved-entries branch January 23, 2019 22:06
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.

No saved entries (list view)

4 participants