First draft of Undo Call Response#305
Conversation
Snackbar action color lightened Changed undo text Changed from toast to snackbar
dektar
left a comment
dektar
left a comment
There was a problem hiding this comment.
Looks really good! If you can add tests that'd be awesome, but I don't think this area has any tests yet anyway so it might not be in scope. Some other comments below.
| Outcome.getDisplayString(this, mPendingOutcome.status)); | ||
| String contactName = mIssue.contacts.get(mPendingContactIndex).name; | ||
| String message = getResources().getString( | ||
| R.string.call_reported_undo_format, outcomeLabel, contactName); |
There was a problem hiding this comment.
can we test this with a long rep name -- that it clips or wraps OK?
There was a problem hiding this comment.
I took Susan's advice and shortened the text (removed the rep's name).
Can definitely add it back if you feel strongly. But with it removed it obviates the need for this comment!
There was a problem hiding this comment.
I think less text and no rep name is good.
| int index = mPendingContactIndex; | ||
| mPendingContactIndex = null; | ||
| mPendingOutcome = null; | ||
| mPendingCallHandler.removeCallbacksAndMessages(null); |
There was a problem hiding this comment.
would we want to removeCallbacksAndMessages even if mPendingContactIndex == null, i.e. do this first in any case?
There was a problem hiding this comment.
yep, good catch.
| Outcome outcome = mPendingOutcome; | ||
| mPendingContactIndex = null; | ||
| mPendingOutcome = null; | ||
| mPendingCallHandler.removeCallbacksAndMessages(null); |
There was a problem hiding this comment.
would we want to removeCallbacksAndMessages even if mPendingContactIndex == null..., i.e. do this first in any case?
| showContactsUi(); | ||
| Intent intent = new Intent(getApplicationContext(), RepCallActivity.class); | ||
| intent.putExtra(KEY_ISSUE, mIssue); | ||
| intent.putExtra(RepCallActivity.KEY_ADDRESS, mAddress); |
There was a problem hiding this comment.
i don't think we need to pass address to RepCallActivity any more.
dektar
left a comment
dektar
left a comment
There was a problem hiding this comment.
LG with some minor comments to address first, but you don't need to send it past me again.
| @@ -0,0 +1,19 @@ | |||
| { | |||
There was a problem hiding this comment.
let's add .claude/ to .gitignore (could be in a separate change, or in this one) and remove this file.
| LinearLayout contentRow = (LinearLayout) actionView.getParent(); | ||
|
|
||
| float density = getResources().getDisplayMetrics().density; | ||
| int sizePx = Math.round(20 * density); |
There was a problem hiding this comment.
prefer to get dimensions from dimens.xml or constants from some values xml or at least const in java, rather than hard-coding numbers.
| <!-- Snackbar shown when the user is returned to the call screen after tapping UNDO [CHAR_LIMIT=40] --> | ||
| <string name="call_response_undone">Call response undone</string> | ||
| <!-- Snackbar shown when the user is returned to the call screen after tapping UNDO [CHAR_LIMIT=25] --> | ||
| <string name="call_response_undone">Not reported</string> |
There was a problem hiding this comment.
maybe "Call unreported" or "Call report cancelled"? I feel like "not reported" is a little less clear.
2 participants
What type of PR is this? (check all applicable)
Description
Related Issues
Were the changes tested?
have not been included