kredits/kredits-web
9
0
Fork 0
Code Issues 22 Pull Requests 1 Actions Releases 18 Activity

Gracefully handle pending changes for contributions #159

Merged
raucao merged 6 commits from feature/pending_changes into master 2019-10-18 15:31:42 +00:00
Conversation 11 Commits 6 Files Changed 9
+46 -12
raucao commented 2019-09-20 21:40:09 +00:00 (Migrated from github.com)
Copy Link
Copy Source

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)
Copy Link
Copy Source

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

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
Copy Link
Copy Source

Why does this property need to be volatile?

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
Copy Link
Copy Source

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.

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)
Copy Link
Copy Source

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

@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
Copy Link
Copy Source

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)
Copy Link
Copy Source

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

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)
Copy Link
Copy Source

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)
Copy Link
Copy Source

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)
Copy Link
Copy Source

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.
Reviewers
No Reviewers
bumi
raucao
galfert
Labels
Clear labels
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/kredits-web#159
Delete Branch "feature/pending_changes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?