821: Manual create #1093

eliserichards · 2019-11-26T23:46:05Z

Fixes #821

Testing and Review Notes

Designs: https://app.zeplin.io/project/5b6895bfe4af825140aa8dbc?seid=5c5490093af38330875be09c

Routes back to the item list after a successful save. Might do the routing to the newly created item on #822.

Screenshots or Videos

To Do

eng todo

other

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
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

jhugman

🚚 🚌 🚗 Drive-by 🚏 🛑

Some things that come out of this:

Naming conventions: can we work out a combination of words to show that there's some relationship between Create, Edit, Show, Item, View, Fragment and Presenter, and then rename all classes to fit that scheme. e.g.

CreateItem + View, Fragment, Presenter.
EditItem + View, Fragment, Presenter.
DisplayItem + View, Fragment, Presenter.

There seems to be a massive similarity between CreateItem and EditItem, all the way up to the discard changes dialog. Can we re-factor EditItem fragment and presenter to allow some re-use? The answer to this may be "I tried, but it wasn't worth it".
Could we start moving things into the ItemDetailStore. I have a feeling we'll get quite a long way by injecting the DataStore into the ItemDetailStore. e.g. how might we do duplicate username detection with the same code path as DisplayItemPresenter (née ItemDetailPresenter).

jhugman · 2019-12-05T16:55:21Z

app/src/main/java/mozilla/lockbox/presenter/AppRoutePresenter.kt

@@ -142,13 +143,15 @@ class AppRoutePresenter(

            R.id.fragment_locked to R.id.fragment_item_list -> R.id.action_locked_to_itemList
            R.id.fragment_locked to R.id.fragment_welcome -> R.id.action_locked_to_welcome
+            R.id.fragment_locked to R.id.fragment_create -> R.id.action_locked_to_manualCreate


We're able to create from the locked screen?

If the app autolocks in the middle of create, do we want to discard everything? My assumption is that we would want to continue where we left off and continue from the create screen until we intentionally ❌ or ✅

eliserichards · 2019-12-05T19:11:05Z

I found the Item in EditItemFragment to be redundant. My preference would be to remove Item from the names altogether and stick with:

Create + View, Fragment, Presenter.
Edit + View, Fragment, Presenter.
ItemList + View, Fragment, Presenter
ItemDetail + View, Fragment, Presenter

That being said, I'm more than willing to be convinced 😌

eliserichards · 2019-12-06T16:43:15Z

We arrived on the following for naming conventions:

CreateItem + View, Fragment, Presenter.
EditItem + View, Fragment, Presenter.
DisplayItem + View, Fragment, Presenter
ItemList + View, Fragment, Presenter

eliserichards · 2019-12-06T20:36:49Z

@jhugman this needs some styling work. Looks like the view isn't applying the correct styles or managing the closeKeyboard functions the way that it should after I refactored the presenters. I would like a pair of eyes on this, or maybe an hour or so to ✨ pair ✨ on Monday if you have some bandwidth 😄

jhugman · 2019-12-09T14:17:34Z

I would like a pair of eyes on this, or maybe an hour or so to ✨ pair ✨ on Monday if you have some bandwidth 😄

We can do both. :)

jhugman

The closeKeyboard problem.

I'm fairly sure we just want to closeKeyboard when we move away from this screen, in which case we can call it from the fragment onPause method or call it from RoutePresenter when there is a change in route action.

app/src/main/java/mozilla/lockbox/presenter/ItemListPresenter.kt

eliserichards · 2019-12-09T18:10:44Z

eliserichards · 2019-12-11T03:18:24Z

@jhugman I fixed the FAB issue, but I still have two outstanding issues (see my checklist in the last comment). Other than that the functionalities seem to be working as expected.

eliserichards

When initially opening the create screen, hostname and password fields default to the error state

**Moved to new issue: ** #1120

changecourse · 2020-01-02T16:51:07Z

Moving a conversation I had with @jhugman in slack to this PR:

essentially, in almost all use cases, a user shouldn't see these inline errors until they've actually made a misstep. I think the error messages should only be visible once someone leaves focus, and we've checked that they hadn't met the requirements.

So as a use case, someone comes to this screen after hitting the + button...

no errors yet present, field focused on web address, keyboard expanded
upon entering a web address and navigating to the next field (username) we do a client-side check on the web address. if it is not meeting requirements we then throw the client-side error to the web address field
same sort of logic for other fields... we assume that someone is not going to type a duplicate or leave a field blank, so we don't show any errors until they've actively demonstrated that they have ignored requirements, by navigating into AND out of a given field

jhugman · 2020-01-02T17:01:04Z

@changecourse I'd like to move this into a follow up issue. This PR is big enough and long running enough to get this landed before more work goes into it.

Add click route for item list create button. Add icon on create button. Tests for item list and create presenter Update credentials to save Cleanup and passing tests Ripple click on + button Password visibility tests Field titles instead of hints Rename edit view. Cleanup presenters and tests Lint and datastore add test

…esenter for create and edit shared functionality. First pass on mutation presenter Refactoring presenters

…ounds and styles correctly for edit and create. Close keyboard onpause and ondestroy. Item list button and drawer working. Rebase and set cursor color on create hostname field Restore sort spinner Fixup EditItemPresenterTest Fixup DisplayItemStoreTest ktlintFormat Refactor error observation. WIP pairing. Moving credentialToSave to an Observable Manual create refactored This brings a lot of the logic from gathering data about the edit session in to the `ItemDetailStore`. * `availableUsernames` * `isDirty` * `isEditing`. This allows us to have a much heavier `ItemMutationPresenter`, which is only lightly specialized by `EditItemPresenter` and `CreateItemPresenter`. With this store, we introduce the concepts of an edit/create session, which the store manages for us. Finish refactoring presenters Begin to unify fragments Trying to get create and edit fragments working together, with separate XML files.

Both create and edit working as expected, with errors Making save more consistent and safer Fixup the correct field to be editing ktlintFormat

Moved tests to use the more heavy weight item detail store, and lighter presenters. We're still not testing the mutate item presenters explicitly, but test coverage seems fairly sane.

jhugman

Elise says it's ok. I say it's ok. Bitrise it's ok. BGB.

eliserichards requested a review from a team as a code owner November 26, 2019 23:46

eliserichards force-pushed the 821-manual-create branch from 8a2bca7 to 06299de Compare December 3, 2019 20:52

jhugman reviewed Dec 5, 2019

View reviewed changes

eliserichards force-pushed the 821-manual-create branch from 741f871 to b15176e Compare December 6, 2019 01:27

eliserichards changed the title ~~[WIP] 821: Manual create~~ 821: Manual create Dec 6, 2019

eliserichards force-pushed the 821-manual-create branch from b15176e to 26fb4a7 Compare December 6, 2019 01:42

jhugman reviewed Dec 9, 2019

View reviewed changes

app/src/main/java/mozilla/lockbox/presenter/ItemListPresenter.kt Outdated Show resolved Hide resolved

eliserichards force-pushed the 821-manual-create branch from 9a606f5 to a0e35c7 Compare December 9, 2019 20:42

eliserichards force-pushed the 821-manual-create branch from 2dd5308 to eb2c2d9 Compare December 11, 2019 03:34

eliserichards commented Dec 30, 2019

View reviewed changes

Elise Richards added 4 commits January 2, 2020 10:59

Manual create fragment, presenter, and xml.

8a13b28

Rename edit to remove redundant \'Item\'. Add routes.

e715027

Add actions for item detail and datastore.

c8890fa

Create button on item list.

7945b64

eliserichards mentioned this pull request Jan 2, 2020

[manual create] Only show inline errors after fields have been changed #1120

Closed

Elise Richards and others added 5 commits January 2, 2020 11:15

Consistent file naming for create, edit, item detail. Add mutation pr…

8317be5

…esenter for create and edit shared functionality. First pass on mutation presenter Refactoring presenters

Stabilize create presenter and fragment

58ec788

Both create and edit working as expected, with errors Making save more consistent and safer Fixup the correct field to be editing ktlintFormat

Refactoring

b253f18

eliserichards force-pushed the 821-manual-create branch from 5626e9d to b253f18 Compare January 2, 2020 23:36

Fix up tests

33c4276

Moved tests to use the more heavy weight item detail store, and lighter presenters. We're still not testing the mutate item presenters explicitly, but test coverage seems fairly sane.

Fixup ui tests

613dac3

jhugman approved these changes Jan 3, 2020

View reviewed changes

jhugman merged commit 0dfe1a1 into master Jan 3, 2020

jhugman deleted the 821-manual-create branch January 3, 2020 17:34

This was referenced Jan 3, 2020

Manual create discard changes #823

Closed

Manual create field validation #822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

821: Manual create #1093

821: Manual create #1093

eliserichards commented Nov 26, 2019 •

edited

Loading

jhugman left a comment

jhugman Dec 5, 2019

eliserichards Dec 5, 2019

eliserichards commented Dec 5, 2019 •

edited

Loading

eliserichards commented Dec 6, 2019

eliserichards commented Dec 6, 2019

jhugman commented Dec 9, 2019

jhugman left a comment

eliserichards commented Dec 9, 2019 •

edited by jhugman

Loading

eliserichards commented Dec 11, 2019

eliserichards left a comment •

edited

Loading

changecourse commented Jan 2, 2020

jhugman commented Jan 2, 2020

jhugman left a comment

821: Manual create #1093

821: Manual create #1093

Conversation

eliserichards commented Nov 26, 2019 • edited Loading

Testing and Review Notes

Screenshots or Videos

To Do

eng todo

other

jhugman left a comment

Choose a reason for hiding this comment

jhugman Dec 5, 2019

Choose a reason for hiding this comment

eliserichards Dec 5, 2019

Choose a reason for hiding this comment

eliserichards commented Dec 5, 2019 • edited Loading

eliserichards commented Dec 6, 2019

eliserichards commented Dec 6, 2019

jhugman commented Dec 9, 2019

jhugman left a comment

Choose a reason for hiding this comment

eliserichards commented Dec 9, 2019 • edited by jhugman Loading

eliserichards commented Dec 11, 2019

eliserichards left a comment • edited Loading

Choose a reason for hiding this comment

changecourse commented Jan 2, 2020

jhugman commented Jan 2, 2020

jhugman left a comment

Choose a reason for hiding this comment

eliserichards commented Nov 26, 2019 •

edited

Loading

eliserichards commented Dec 5, 2019 •

edited

Loading

eliserichards commented Dec 9, 2019 •

edited by jhugman

Loading

eliserichards left a comment •

edited

Loading