address delete functionality #2128
Conversation
|
It looks like this contributor signed our Contributor License Agreement. 👍 Many thanks, Ethcore CLA Bot |
|
I'm not entirely happy with bf4b92c. I get the idea of being able to easily switch between themes/ui frameworks. But is it really worth the abstraction? How often are we going to switch the UI theme? Does it make sense to abstract everything away into React components? I feel like this is making the code easier, but not simpler. |
| } | ||
| }); | ||
| Object.keys(accountsInfo) | ||
| .map((address) => Object.assign({}, accountsInfo[address], { address })) |
There was a problem hiding this comment.
Use Object.entries here? :)
| .map((address) => Object.assign({}, accountsInfo[address], { address })) | ||
| .filter((account) => !account.meta.deleted) | ||
| .forEach((account) => { | ||
| if (account.uuid) { |
There was a problem hiding this comment.
Am I missing something here or does that not make sense? 😁
There was a problem hiding this comment.
uuid == null - it is an address
uuid != null - it is an account
We get one list of everything via personal_accountsInfo
There was a problem hiding this comment.
Just wondering because the if and else blocks are the same.
There was a problem hiding this comment.
Lemme just check that again, they didn't used to be the same - I think I probably went only half-way with cleaning/simplifying things then.
There was a problem hiding this comment.
if (account.uuid) {
accounts[account.address] = account;
} else {
contacts[account.address] = account;
}
One moves it to the accounts bucket, the others to the contacts bucket. (As soon as we have https://github.com/ethcore/parity/issues/2115 - there will probably be a contracts bucket as well)
| return [ | ||
| <Button | ||
| label={ labelNo || 'no' } | ||
| icon={ iconNo || <ContentClear /> } |
There was a problem hiding this comment.
Minor grumble here:
ContentClear will even get imported & bundled if never actually use it. Unfortunately having no default icon is not very consumer-friendly either.
| // You should have received a copy of the GNU General Public License | ||
| // along with Parity. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| export default from './confirmDialog'; |
There was a problem hiding this comment.
Too keep the code consistent and small pitfalls rare, I guess from './ConfirmDialog' is better.
There was a problem hiding this comment.
We are consistent everywhere here - have not been using uppercase names for class files. (Which is a slight departure from previous work, i.e. Signer/Status). Changing this one mens changing all.
| ]).isRequired, | ||
| visible: PropTypes.bool.isRequired, | ||
| onNo: PropTypes.func.isRequired, | ||
| onYes: PropTypes.func.isRequired |
There was a problem hiding this comment.
I'd prefer onConfirm and something like onReject or onDeny here. It is more expressive than "yes" and "no". Same for labelYes and labelNo.
| </div> | ||
| </div> | ||
| <div className={ styles.description }> | ||
| { contact.meta.description || 'this is just some long description to make me feel happy about myself and to see what actually happens and just maybe we should throw some wrapping in here as well' } |
There was a problem hiding this comment.
replace this fallback with something like "this contact has no description"?
There was a problem hiding this comment.
I actually removed that - it was for debug. Not sure why it is not in.
| const { address } = this.props.params; | ||
| const contact = (contacts || {})[address]; | ||
|
|
||
| this.toggleDeleteDialog(); |
There was a problem hiding this comment.
In this place, it always hides the delete dialog, so I'd prefer hideDeleteDialog & showDeleteDialog, as they're more expressive. Reading toggleDeleteDialog, I had to stop and actually figure out if it makes sense.
derhuerst
added
A5-grumble 🔥
* js: use standard modal title functionality fix modal title size where no steps are available get adding addresses working again (context migration) fix merge conflict fix grumble use only one image for tokens, scale it (maps what is available when pulled form IMG meta in TokenReg) unknown image when none found for a token don't show tokens.value === 0 in transfer cater for the cases where the token doesn't have an image token images show up in Signer again with button images, trial header title updates add support for buttons fix calculation where transfer value is non-ETH IdentityIcon show up on the send button token icons show up in transaction list transaction list powered by # Conflicts: # js/src/ui/ConfirmDialog/confirmDialog.css # js/src/ui/Modal/modal.js
jacogr
added
A0-pleasereview 🤓
| } | ||
|
|
||
| if (different) { | ||
| if (JSON.stringify(this._lastInfo) !== JSON.stringify(info)) { |
There was a problem hiding this comment.
I think comparing to JSON.stringify()s is pretty dangerous. If the order of properties is different, this will fail. Also JSON.stringify is usually slower than deepStrictEqual implementations and it breaks with circular structures. JSON.stringify might also fail on objects with a prototype.
deep-equal is very lightweight.
There was a problem hiding this comment.
Will log an issue to swap.
Functionality in action - https://www.youtube.com/watch?v=aQjHQsJFRso
Add the ability to delete (or rather mark addresses as deleted) for the address book. The same would and should apply for contracts (when we get there).
(We should be pulling in https://github.com/ethcore/parity/pull/2101 soon, since adding of addresses is completely broken atm, that branch fixes the functionality)