search

newlandsvalley / chord-editor

UI for generating simple chord diagrams for guitar, bass and piano
MIT License
10 stars 3 forks source link

Add support for tenor guitar #2

Closed jgarte closed 2 years ago

jgarte commented 2 years ago

Hi, this is a Draft for #1.

I still haven't tested the following:

newlandsvalley commented 2 years ago

Many thanks for resolving these. I'll try to review it over the weekend.

newlandsvalley commented 2 years ago

I think this is everything I've found wrt to the files you've added. Could you also 1) amend the purescript version in devDependencies in package.json to 0.15.4 (and also remove pulp which is no longer used)? and 2) could we add something to README.md to say we also support tenor guitar? I don't think we need further PNG pictures of these guitar variants, just some sort of comment that these are also in the mix.

Update: I've had to edit package.json myself to add a dependency on esbuild in order to get CI to work. So I've made the edits myself to this file as requested above. You will thus probably need to refresh from Master.

jgarte commented 2 years ago

I think this is everything I've found wrt to the files you've added. Could you also 1) amend the purescript version in devDependencies in package.json to 0.15.4 (and also remove pulp which is no longer used)? and 2) could we add something to README.md to say we also support tenor guitar? I don't think we need further PNG pictures of these guitar variants, just some sort of comment that these are also in the mix.

Update: I've had to edit package.json myself to add a dependency on esbuild in order to get CI to work. So I've made the edits myself to this file as requested above. You will thus probably need to refresh from Master.

@newlandsvalley Thanks for the review.

Update: I pushed some new commits with all your requests above.

jgarte commented 2 years ago

Thanks for reviewing and merging this! Much appreciated.