-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Further chord picker functionality and story #27
base: master
Are you sure you want to change the base?
Conversation
Hi @RyanButton! Thanks a lot for this!! I am super excited. However I'm a bit busy lately and it'll take me some time to go through it and your previous pull request. I'll let you know as soon as I have the time. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @RyanButton,
I made some initial comments. Also, I believe you included a package-lock json by accident. Are you using npm
instead of yarn
?
I'll review the logic in the next few days and let you know.
Thanks!
.vscode/settings.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ | |||
"typescript.tsdk": "node_modules\\typescript\\lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you commited this file by accident.
Thanks for the comments @4lejandrito. This PR needs a rebase to get newest changes from master (first PR you merged), and I also did mistakenly use npm instead of yarn (force of habit). Will fix her up in the next couple of days. Feel free to keep chugging along on reviewing the logic etc and will apply changes to that as well 🙂 |
6327003
to
811fc6b
Compare
54f4336
to
eebe030
Compare
@4lejandrito I've rebased, removed the stray files, as well as squashed the commits. Feel free to review at your own leisure. Cheers 👍 |
Hi @RyanButton, I've come back to this. Would you mind adding a few tests to fretter.test.ts? Also, if you're into React, what about surfacing this functionality to the UI? Somewhere in this screen: |
Thanks for the feedback @4lejandrito, I will go through and have a crack at implementing some of the functionality/improvements you've mentioned. Little bit busy myself lately (first child was just born) so may take longer than usual. |
Oh man congratulations!! I also had my first kid back in February, that's why it's taking me so long to review this 😅 . It's a wonderful experience, enjoy it, time flies. |
724b8b2
to
9540330
Compare
Hi @4lejandrito, implemented those suggestions. Fire at will 🙂 |
Hi! I was testing this and found that the numbers don't really match... If you pick a C Major and pick "All" you get 19 possible chords, If you pick Open you get 2, and if you pick Barre you get 4... shouldn't Barre + Open = All? |
3282ea8
to
45e813a
Compare
Hello there,
This PR contains implementation for the following tasks on Issue #8:
As well as this, I have added a story to the story book for testing the chord picker.
It's also worth noting that there are some issues within here that I believe are related to the core of the chord picker code, such as certain 'diminished triad' chords not being picked up.
The intention here is a first pass, then on acceptance
restrictChordTypeTo
,getChordSemitones
etc can be expanded upon.