Skip to content
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

refactor: correctly sort wallets and contacts in overview and sidebar #1044

Merged
merged 14 commits into from Feb 7, 2019

Conversation

@dated
Copy link
Contributor

commented Jan 30, 2019

Proposed changes

This fixes the sorting by name of the wallets and contacts in the grid view, table view and sidebar.

It also changes the implementation of the sortByProp (now sortByProps) util, which now uses natural sorting by default and allows for multiple properties.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)

Checklist

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

dated added some commits Jan 30, 2019

@dated dated changed the title [WIP] refactor: correctly sort wallets and contacts in overview and sidebar refactor: correctly sort wallets and contacts in overview and sidebar Jan 30, 2019

dated added some commits Jan 30, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

As discussed with Alex, wallets without a name (custom or from the Network) will stay at the top of the list.

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

You should probably fix this too:

screenshot 2019-02-04 at 19 28 04

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Does that happen with these changes @zillionn ?

@zillionn

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Yes.

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

Thanks for the feedback, I'll take a look asap!

dated added some commits Feb 4, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

should be fixed now @zillionn, stupid typo 😉

@j-a-m-l j-a-m-l self-requested a review Feb 5, 2019

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

@dated Your pull request doesn't have a test case, which is a requirement for it to be merged. Please provide it and one of the developers will review it before merging.

@j-a-m-l
Copy link
Contributor

left a comment

@dated everything looks OK, but at least sortByProps should be tested

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

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

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Done as requested @j-a-m-l

@j-a-m-l
Copy link
Contributor

left a comment

Could you change the tests to make them more comprehensible and complete?

One way to do it would be creating specific tests to how empty values or numbers are ordered. Without tests like that, it may look that it's accidental, not on purpose.

@dated dated force-pushed the dated:natural-sort branch from f56b561 to 8108715 Feb 5, 2019

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Better?

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thanks @dated. Now it's a lot easier to read and it's also easier to grasp what should be expected when sorting.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@dated I just realized that the Ledger wallets are also visible when navigating through the contacts page, so the sidebar is showing both, contacts and owned Ledger wallets. Would you mind to change it?

@dated

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I've made some changes to how the wallets are fetched, I can't test it myself right now unfortunately.

dated and others added some commits Feb 6, 2019

@j-a-m-l

j-a-m-l approved these changes Feb 7, 2019

Copy link
Contributor

left a comment

Thanks, @dated

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 7, 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!

@j-a-m-l j-a-m-l merged commit b88f448 into ArkEcosystem:develop Feb 7, 2019

1 check passed

ci/circleci: test-node-9 Your tests passed on CircleCI!
Details
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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

@dated dated deleted the dated:natural-sort branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.