Gracefully handle pending changes for contributions #159

2019-09-20T21:40:09Z

raucao commented

2019-09-20 21:40:09 +00:00

(Migrated from github.com)

This adds the pending tx data to pending contributions (after adding or vetoing, until the tx is mined). It also disables the veto button while pending.

WIP until I've added the same for contributor additions/changes. Also, it's branched out from #156, ~~which hasn't been merged yet.~~

closes #120

This adds the pending tx data to pending contributions (after adding or vetoing, until the tx is mined). It also disables the veto button while pending. WIP until I've added the same for contributor additions/changes. Also, it's branched out from #156, <del>which hasn't been merged yet.</del> closes #120

bumi (Migrated from github.com) reviewed 2019-09-20 21:40:09 +00:00

raucao commented

2019-10-09 12:52:38 +00:00

(Migrated from github.com)

Looking for reviews now. Doing contributors in a separate PR.

galfert (Migrated from github.com) reviewed 2019-10-09 16:40:05 +00:00

app/controllers/dashboard/contributors/show.js

galfert (Migrated from github.com) commented

2019-10-09 16:40:04 +00:00

Why does this property need to be volatile?

raucao (Migrated from github.com) reviewed 2019-10-10 15:10:08 +00:00

app/controllers/dashboard/contributors/show.js

raucao (Migrated from github.com) commented

2019-10-10 15:10:08 +00:00

I thought that's just how you do computed properties without dependency now (as someone told me about this). Already thought it doesn't sound right, when the property never changes, but haven't looked it up.

raucao commented

2019-10-10 15:10:58 +00:00

(Migrated from github.com)

@galfert Does your comment mean that you took this for a spin?

galfert (Migrated from github.com) reviewed 2019-10-11 11:22:15 +00:00

app/controllers/dashboard/contributors/show.js

galfert (Migrated from github.com) commented

2019-10-11 11:22:15 +00:00

No, marking a property as volatile just deactivates caching and makes it recalculate on every access. It also prevents property notifications.

The volatile() modifier got deprecated recently. So I think just using an empty string as the dependency would be better here.

No, marking a property as volatile just deactivates caching and makes it recalculate on every access. It also prevents property notifications. The `volatile()` modifier [got deprecated recently](https://deprecations.emberjs.com/v3.x/#toc_computed-property-volatile). So I think just using an empty string as the dependency would be better here.

galfert commented

2019-10-11 11:22:47 +00:00

(Migrated from github.com)

I only looked at the code. Besides my one comment it looked ok to me.

raucao commented

2019-10-11 12:18:12 +00:00

(Migrated from github.com)

Could you please test it?

(If you still have trouble running this easily on your machine, then you need to tell us that. Everything should work in a way that it's very easy to test now.)

Could you please test it? (If you still have trouble running this easily on your machine, then you need to tell us that. Everything should work in a way that it's *very* easy to test now.)

raucao commented

2019-10-18 07:49:46 +00:00

(Migrated from github.com)

Note to whoever didn't use this feature before: in order to get longer, more realistic confirmation times from your dev chain, you can add block-time to the devchain npm script, like so:

-    "devchain": "aragon devchain --port 7545",
+    "devchain": "aragon devchain --port 7545 --block-time 20",

Note to whoever didn't use this feature before: in order to get longer, more realistic confirmation times from your dev chain, you can add `block-time` to the `devchain` npm script, like so: ```diff - "devchain": "aragon devchain --port 7545", + "devchain": "aragon devchain --port 7545 --block-time 20", ```

galfert commented

2019-10-18 14:41:56 +00:00

(Migrated from github.com)

I tested the PR and confirmed everything to work.

I fixed the merge conflict and changed the confirmation-status helper to detect the pending changes. I also removed the volatile() property modifier everywhere, as it was already throwing a deprecation warning.

I tested the PR and confirmed everything to work. I fixed the merge conflict and changed the `confirmation-status` helper to detect the pending changes. I also removed the `volatile()` property modifier everywhere, as it was already throwing a deprecation warning.

🎉 1

galfert (Migrated from github.com) approved these changes 2019-10-18 14:42:44 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/kredits-web#159