-
Notifications
You must be signed in to change notification settings - Fork 530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added AddressBook feature #2414
base: master
Are you sure you want to change the base?
Conversation
@asif-finimble this is a very large PR for a small feature. Can we prune it a little to get the code change down a little. I can see a few wins in the duplicated resources, there are a few strings which are already translated. Also, the adapter sliding has already been written, no need for a new class to do the same thing. Please see TokensAdapter which uses the token sliding (if that's what it's for - I might have mis-read it). Let's work together after I get back from the conference to work out how to reduce the code size; it looks like a good implementation but I'm pretty sure we can re-use a lot of existing components rather than writing new ones that duplicate function. |
OK. I went through the code and the sliding classes are used as inner classes so they are tightly coupled. It wont work with AddressBook. |
Ok, but then that slider should be moved out of the class rather than duplicating code. Can probably just move the slider inner class out verbatim as we know it's reliable (unless it accesses private vars, in which case it'll need a new callback interface). Adding AddressBook should not need to touch 58 files. It should only operate on InputAddress, possibly a BottomSheetDialog. I don't think it's been fully designed yet. Let me get back to you on this one. I agree with your implementation it should be a Realm item rather than SharedPrefs. |
The sliding class is only extending As for the AddressBook, I found that the AddressBook feature was not implemented. So first I implemented the AddressBook feature by referring the zeplin designs and after that integrated it with |
@JamesSmartCell I resolved the conflicts. |
/** | ||
* This is associated ETH name of #walletAddress | ||
*/ | ||
private String ethName; |
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.
Do you mean ens
name?
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.
yes
} | ||
catch (Exception e) | ||
{ | ||
e.printStackTrace(); |
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.
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.
Updated logging with Timber.
.equalTo("walletAddress", oldContact.getWalletAddress()) | ||
.findFirst(); | ||
if (realmContact != null) { | ||
// realmContact.setName(newContact.getName()); |
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.
The comment can be removed.
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.
done
} else { | ||
mode = ADDRESS_OPERATION_MODE.EDIT; | ||
setTitle(getString(R.string.title_edit_address)); | ||
contactToEdit = getIntent().getParcelableExtra(C.EXTRA_CONTACT); |
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.
Duplicated.
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.
fixed
String name = inputViewName.getText().toString().trim(); | ||
boolean isDataValid = true; | ||
if (address.isEmpty()) { | ||
inputViewAddress.setError("Address should not be empty"); |
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.
i18n
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.
done
data.add(position, item); | ||
notifyItemInserted(position); | ||
} catch(Exception e) { | ||
} |
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.
Log error.
app/src/main/java/com/alphawallet/app/ui/widget/adapter/AddressBookAdapter.java
Show resolved
Hide resolved
app/src/main/java/com/alphawallet/app/viewmodel/AddEditAddressViewModel.java
Show resolved
Hide resolved
app/src/main/java/com/alphawallet/app/viewmodel/AddEditAddressViewModel.java
Show resolved
Hide resolved
app/src/main/java/com/alphawallet/app/viewmodel/AddressBookViewModel.java
Outdated
Show resolved
Hide resolved
@seabornlee, code updated with changes. |
LGTM, thanks a lot! |
e967340
to
32f6049
Compare
a1cfb4f
to
8acd3f7
Compare
Closes #1672
@JamesSmartCell I have implemented AddressBook feature and added it to InputAddress component. Here are the details regarding the implementation. Please let me know if I need to change/update any thing.
AddressBook Activity
AddEditAddress Activity
Modes: Add & Edit
Add
Edit
Input Address Component
TransactionDetailActivity
Activity Adapter