-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
74ede49
to
eafecce
Compare
d30f8cf
to
62e0445
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have requested some changes.
I'd also like to see some screenshots to demonstrate the changes, and make it easy for @changecourse to review.
Thanks.
|
||
view.usernameFieldClicks | ||
.subscribe { | ||
view.setTextSelectionOnPasswordToggle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing? This looks weird. Could you comment it?
Not sure it's quite as useful as we might think.
@@ -98,6 +102,11 @@ class EditItemPresenterTest { | |||
override fun setSaveEnabled(enabled: Boolean) { | |||
_saveEnabled = enabled | |||
} | |||
|
|||
override fun setTextSelectionOnPasswordToggle() { | |||
// make sure the text selector is at the end of the line when password is toggled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH HA! That's what this does. Could you rename the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
62e0445
to
88297cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good. I'm happy for this to land, from a code point of view.
@changecourse what say you?
app/src/main/java/mozilla/lockbox/presenter/EditItemPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/mozilla/lockbox/presenter/EditItemPresenter.kt
Outdated
Show resolved
Hide resolved
@@ -62,6 +62,8 @@ class EditItemFragment : BackableFragment(), EditItemDetailView { | |||
} | |||
|
|||
private fun updatePasswordVisibility(visible: Boolean) { | |||
view?.inputPassword?.setSelection(view?.inputPassword?.length() ?: 0) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much better putting this here.
6060690
to
ab0937e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with build #5616 on Pixel 2, Android 9
Upon beginning my review, I ran into a security PIN loop where, after leaving my phone with the build running long enough to time out, the app then prompted me for unlocking (via PIN)... which was expected. What wasn't expected, was having to enter my pin ~10 times before it would actually unlock and return me back to the login list.
Anyways, onto testing the specifics of this PR:
Global
- Padding between titles in header bar is inconsistent (Login List, Login Detail, Edit Login)
Login Detail
- The tap line for web address is thinner than the tap line for username and password
- Horizontal tap lines should extend the full width of the placeholder line, when tapped to copy or open a web address. The same should be true on edit (for the password field)
- Still need an issue for metadata at base of container if one isn't created
Edit Login
- The content area is slightly smaller (white background) when tapping into Edit from the login detail
- The horizontal lines for web address and username move up slightly (upon tapping edit) whereas the horizontal line for password moves down slightly
- The dots change (possibly swapping from monotype to something else?) between views. They should both be monotyped.
- Flash of back arrow upon tapping "Edit" before switching to Close X
FixingLogin Detail
Edit Login
Questions etc
Checking to see if this is just on my branch or not.. please hold
We can either have support for long passwords and shorter horizontal tap lines, or have long passwords overlap the buttons and the tap lines extend the whole length of the field
Not sure what this means 🤔 I thought I left a comment on the issue but I must have forgotten. My b
Known issue. There's an open issue for it somewhere.. please hold |
Addressed above comments. Remaining items can be addressed in other issues
* Clear focus and close keyboard when clicking outside of edittext. * Cleanup and lint * Add new fields to edit test * Rename text selection method. Clean up padding on item detail card view. * Refactor keyboard caret selection for password toggling. * Whitespace * Match item detail and edit view padding
Fixes #991
Testing and Review Notes
Putting screenshots in a comment further down so I don't clutter this up too much...
Screenshots or Videos
Taps on edit don't highlight the
hostname
underline anymore (not much to see here...)Selecting username moves cursor to the end. Toggling password moves cursor to the end.
Dismiss keyboard easily
Todo
item detail:kebab menu transitionsmove kebab menu 4dp from sideclick listeners not registering correct item idsMoved to Replace PopupMenu with PopupWindow in detail view #1077
misc:
Done
if possible, match width of edit menu popup to spec (128dp) and only grow if needed for localizationhorizontal line tap state (thick purple line) needs to go all the way across. currently it is stopping where the icon bounding box beginsthe horizontal line tap state (thick purple line) needs to completely overlap the default grey line; currently it is resting slightly above the grey lineThere's a square white background on the cursor tap indicator (when first tapping into a field)Tapping anywhere in the edit view that isn't a field (in the white space of the card or the grey background) selects the line for Name (which should be a disabled field)sans-serif
while the password ismonopace
icons needed for both edit and deleteposition of dropdown should be over the top of the ellipsis, not belowwidth of dropdown should be 128dp (grow to expand if needed for longer localized strings)shadow is stronger in design spec (8dp)confirm label size is 12splabels need letter spacing of 0.4sp -> using the same formatting as item detail view“Delete Entry” requires top and bottom padding (40dp on top, 30dp on bottom)confirm color of Delete text is #d70022Web Address label color should be violet70form field selected line should be violet70form field selected line and base line of web address field should be one and the same (not 2 different lines)fields "jumping" with error statesTo Do