-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
mapping for the Numark Mixtrack Pro FX #4160
Conversation
Thanks for contributing! Based on 2.3 and already accomplished by a PR for the manual, great 👍 As a First-time contributor we need to ask your to sign our CLA. No big deal, hopefully. Please comment here when done. |
Can I use my nickname in the agreement? Or do I need to use real name? |
Most of us have provided their real names. The list is not public and will not be published anywhere. @daschuer Any advice? |
well, the names is listed in the About box, so decide for yourself if you prefer a nick or you real name. the actual purpose is to |
I've submitted the CLA (with nickname). |
I think it is most important that you provide an email address that you will respond to in the future if we ever need to contact past contributors. |
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.
Thanks for submitting your mapping.
Please make sure to install pre-commit for both the mixxx and the manual repository and run it on the files you are submitting.
Co-authored-by: Swiftb0y <[email protected]>
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.
Thanks. Last two comments. After that I'll have another look at the manual page and then thismapping is perfectly mergable (assuming no one else wants to have another look).
Is some action still required of me? Or is the code ready to be merged? |
Don't worry, i think @Swiftb0y just needs to give his final approval and we need to proofread the manual changes (because we want to always merge both PRs at the same time). |
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.
Thank you, mapping looks very good.
@uklotzde I think you requested changes. Please hVe a look and let us know if you're okay with mergingt this. |
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 don't remember that I requested changes 😅 Probably already resolved.
Let's merge the current version as is if everyone agrees as a baseline.
Did you read this new form post?
https://mixxx.discourse.group/t/numark-mixtrack-pro-fx/19561/57
I am unsure if it still applies or if this only affects a previous version.
This should be merged by someone who is also able to merge the corresponding manual PR. |
@uklotzde do you not have write access to the manual repository? |
No |
Thank you for working on this and following through with the code review comments! :) And thanks @Swiftb0y for doing the review. |
Thanks for the in-depth review. I will keep an eye on Mixxx updates and follow up with another PR when it will be possible to properly map FX selection. |
@Be-ing Next time please stash the commits for controller mappings when merging to avoid flooding the Git history of the upstream repo. |
Do you mean squash? I agree that would be appropriate but I have explicitly been told to not do that before. |
IIRC we discussed somewhere that squash-and-merge for controller mappings is appropriate, squashing for C++ changes is not. |
Of course "squash", not "stash" 🙈 |
+60 tiny commits for a single controller mapping is just too much. |
Hi,
I've created a mapping for Mixtrack Pro FX.
The mapping was discussed in this thread and received some good feedback. It was originally created by @bad1dea5 (see here), then modified by me, with some help from @photoenix and others.
Manual PR