Skip to content
This repository has been archived by the owner on Mar 23, 2023. It is now read-only.

feat: add remote sorting to delegate table #1080

Merged
merged 3 commits into from Feb 16, 2019
Merged

feat: add remote sorting to delegate table #1080

merged 3 commits into from Feb 16, 2019

Conversation

dated
Copy link
Contributor

@dated dated commented Feb 15, 2019

Proposed changes

Adds remote sorting to the delegate table. Replaces #763

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes

@ghost
Copy link

ghost commented Feb 15, 2019

@dated Thanks for submitting this pull request, a maintainer will get back to you shortly!

@ghost
Copy link

ghost commented Feb 15, 2019

@alexbarnsley @j-a-m-l @luciorubeens - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added Status: Needs Review The issue or pull request needs a review by a developer of the team. Complexity: Low Less than 64 lines changed. Type: Feature The issue is a request for new functionality. labels Feb 15, 2019
j-a-m-l
j-a-m-l suggested changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@j-a-m-l j-a-m-l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking on the names of the columns doesn't sort them (tested on mainnet and devnet): the request always uses rank:asc.

src/renderer/services/client.js
* @return {Object[]}
*/
async fetchDelegates ({ page, limit } = {}) {
async fetchDelegates (options = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dated , I'm curious, why do you prefer using options instead of destructuring the object here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for conformity in regards to the other methods, such as fetchWalletTransactions.

@ghost ghost added the Status: Needs Changes The pull request needs additional changes before it can be merged. label Feb 15, 2019
@ghost
Copy link

ghost commented Feb 15, 2019

@dated Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@j-a-m-l j-a-m-l added Bounty: Tier 2 Awarded for large features, refactorings, improvements. This is valued at 100 USD. and removed Status: Needs Changes The pull request needs additional changes before it can be merged. Status: Needs Review The issue or pull request needs a review by a developer of the team. labels Feb 16, 2019
@ghost
Copy link

ghost commented Feb 16, 2019

@dated A member has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@ghost ghost added the Status: Member Approved The pull request has been approved by a member. label Feb 16, 2019
@j-a-m-l j-a-m-l merged commit 6704056 into ArkEcosystem:develop Feb 16, 2019
@ghost
Copy link

ghost commented Feb 16, 2019

@dated Your pull request has been merged and marked as tier 2. It will earn you $50 USD.

@j-a-m-l
Copy link
Contributor

j-a-m-l commented Feb 16, 2019

Thanks, @dated

@dated dated deleted the remote-delegates branch February 16, 2019 09:52
@ghost ghost mentioned this pull request Feb 18, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@j-a-m-l j-a-m-l j-a-m-l approved these changes

@alexbarnsley alexbarnsley Awaiting requested review from alexbarnsley

@luciorubeens luciorubeens Awaiting requested review from luciorubeens

Assignees
No one assigned
Labels
Bounty: Tier 2 Awarded for large features, refactorings, improvements. This is valued at 100 USD. Complexity: Low Less than 64 lines changed. Status: Member Approved The pull request has been approved by a member. Type: Feature The issue is a request for new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dated @j-a-m-l