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

feat: add compare function #266

Merged
merged 5 commits into from
Jul 19, 2021
Merged

feat: add compare function #266

merged 5 commits into from
Jul 19, 2021

Conversation

danrivett
Copy link
Contributor

@danrivett danrivett commented Jul 18, 2021

First up - great library - v2 came along at just the right moment as we were starting to look for a good Money TS solution this week. So thank you!

This PR adds another comparison function - compare - to simplify sorting arrays or other collections of Dinero objects.

I did my best to implement this according to existing conventions, but I'm still new to the library, so if you're happy with the intent of this PR but need changes made, please list them all and I'll update the PR. Alternatively feel free to merge and then fix it all!

I gave an attempt at also adding docs for it - as your docs are great - but almost certainly didn't do it right, but I at least wanted to try. Again feel free to send me a list of changes to make, or merge and take care of it yourself if easier.

Finally if this PR is unnecessary due to my ignorance, please let me know. I basically want a simple way to pass a function into Array.prototype.sort() and it seemed sensible to me to provide that in the library rather than create a separate utility function.

PS - The functionality this PR uses is existing Calculator functionality, since it already had a compare function that acted as required - so it just needed the wrapper functions to expose it (which I based off of the existing lessThan comparison function), and now I can sort arrays of Dinero objects like:

import { compare } from 'dinero.js';

const pricesLowToHigh = prices.sort((price1, price2) => compare(price1, price2));

@vercel
Copy link

vercel bot commented Jul 18, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/dinerojs/dinerojs/5v9QZ6TUhJBRb2ePm11bYXjr76vY
✅ Preview: https://dinerojs-git-fork-abelmkr-add-compare-dinerojs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 18, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b9b7e8a:

Sandbox Source
@dinero.js/example-cart-react Configuration
@dinero.js/example-cart-vue Configuration
@dinero.js/example-starter Configuration

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Thank you @danrivett for this contribution. A compare function definitely belongs to Dinero.js!

You did an excellent job with this PR, everything is really clean. I've added GitHub suggestions that you can directly apply, but it's mostly wording for documentation and JSDoc blocks.

I've also enabled the CI for forked pull requests so starting on your next commit you should see more checks (tests, linting, etc.)

Let me know if I can help!

website/data/docs/api/comparisons/compare.mdx Outdated Show resolved Hide resolved
website/data/docs/api/comparisons/compare.mdx Outdated Show resolved Hide resolved
website/data/docs/api/comparisons/compare.mdx Outdated Show resolved Hide resolved
website/data/docs/api/comparisons/compare.mdx Show resolved Hide resolved
packages/core/src/utils/__tests__/compare.test.ts Outdated Show resolved Hide resolved
packages/core/src/utils/__tests__/compare.test.ts Outdated Show resolved Hide resolved
packages/core/src/utils/__tests__/compare.test.ts Outdated Show resolved Hide resolved
packages/dinero.js/src/api/compare.ts Outdated Show resolved Hide resolved
packages/dinero.js/src/api/compare.ts Outdated Show resolved Hide resolved
@danrivett
Copy link
Contributor Author

danrivett commented Jul 18, 2021

Thanks @sarahdayan for the quick review. I applied the changes directly in GitHub, and then just edited one to replace "lesser than" with "less than" as "less than" is the correct form I believe, and is certainly the much more common form.

It may be worth updating other existing occurrences of "lesser than" too to read better. I had thought about that at the time and was going to suggest that in a separate PR.

I haven't yet updated the docs to add another example, as I have to go out this morning, but I will do that later after checking I didn't break the CI with my code review actions commit.

@sarahdayan
Copy link
Collaborator

@danrivett Good call on "less than", indeed "lesser than" isn't the right form in this context. I'll update the docs separately.

Once you add the example, we're good to go! Don't hesitate to tell me if you don't have time and I'll take care of it.

But replaced 'lesser than' with 'less than'.

Co-authored-by: Sarah Dayan <dayan.sarah@gmail.com>
@danrivett
Copy link
Contributor Author

danrivett commented Jul 18, 2021

@sarahdayan I've added an example. Please can you review and I'll make any improvements as necessary. Thanks.

I also rebased it on the latest main branch.

Update: Alternatively, if easier for you because of timezone differences (I'm in Pacific time) feel free to make any fixes yourself and merge it. I'll be back in a couple of hours to fix anything, but I'm more than happy for you to address anything outstanding as needed.

@danrivett
Copy link
Contributor Author

One thing I was excited about was that I saw your tweet the other day (https://twitter.com/frontstuff_io/status/1415781368336900101) about how you can play with Dinero from the docs page's web console, so I thought this would be a great way to verify my code sample, since the PR was automatically deployed to Vercel.

Unfortunately although I see my docs change, it looks like it's using it's not using the version of dinero.js code from the PR but I assume the latest release?

trying-to-test-sort-example

I have no idea how feasible it would be to have it use the PR version of the library so I can test it in the browser, or if there's a better way? I did look at codesandbox.io but I don't think it allows me to test in browser in a sandbox, but let me know if so.

This is a minor thing, but thought I'd mention it as thought it was cool you can test our dinero.js from the web console.

@sarahdayan
Copy link
Collaborator

@danrivett The documentation playground uses the latest release from jsDelivr. Technically, we already build the library when deploying the site to get the build sizes, so when the branch isn't main, we could drop it in there with the site's assets and load it instead. Next fun stuff to build!

You can, however, use the CodeSandbox starter playground that was built in this PR (this one, see this comment) and test it there using the console. CodeSandbox builds artifacts and symlinks them like with Yarn workspaces, so that's a good way to test things out as users would.

@danrivett
Copy link
Contributor Author

Ah thanks for the pointer on how to test with codesandboxio, that's really helpful. I will check that out later as just left for a couple of hours.

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Unfortunately, GitHub doesn't allow edits of PRs from maintainers on organization repositories (see isaacs/github#1681), so I can't push directly on your branch.

Once you apply this (simplified example) we're good.

website/data/docs/api/comparisons/compare.mdx Outdated Show resolved Hide resolved
Co-authored-by: Sarah Dayan <dayan.sarah@gmail.com>
@danrivett
Copy link
Contributor Author

All done @sarahdayan ! If there's anything else you spot let me know, but hopefully should be good to go.

Copy link
Collaborator

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

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

Awesome! Let's ship it 🚀

@sarahdayan sarahdayan merged commit 53f84c2 into dinerojs:main Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants