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

added mbncp MIDI JSFX #29

Merged
merged 3 commits into from
Oct 1, 2017
Merged

added mbncp MIDI JSFX #29

merged 3 commits into from
Oct 1, 2017

Conversation

nofishonfriday
Copy link
Contributor

No description provided.

@cfillion
Copy link
Member

cfillion commented Jun 9, 2017

Looking good, I just hope the original author doesn't mind having his old FX uploaded here.

I have three remarks:

  • Changelog is not mandatory and repeating the version number in it is redundant.
  • The display name of "PitchWheel Control Center v0.2(click edit for help)" should probably just be "PitchWheel Control Center" for prettiness.
  • I think "Note duration control" should mention its Legato purpose, beside now it looks similar to my own "MIDI Note Length Control" which does a different thing.

@nofishonfriday
Copy link
Contributor Author

updated PR according to your comments

I've kept the changelog (but stripped the version number). Thinking in case it gets updated sometime in the future, wouldn't this make sense ?

You're right, it's nicer asking the author beforehand if it'sok adding to ReaPack. I'll PM him, so you can wait with merging until I get reply (will post update here if I do).

@nofishonfriday
Copy link
Contributor Author

nofishonfriday commented Jul 6, 2017

It's now about a month that I sent mbncp (author of these JSFX) a PM asking about adding them to ReaPack with no reply. His last forum login was 08-03-2015. Seems like he isn't active on the forum anymore. Not sure what to do. I opened this pull request because I think these are worth adding to ReaPack and not getting buried in some random forum thread (as I find them useful personally).

As I don't know what's ReaTeam's policy in this case, I'll leave it to you how to proceed. (no problem, if it gets closed without merging).

@cfillion cfillion merged commit 82ceb72 into ReaTeam:master Oct 1, 2017
@nofishonfriday
Copy link
Contributor Author

Thanks.

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.

2 participants