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

Fix settings for custom control sensitivities #37

Merged
merged 3 commits into from
Feb 7, 2023

Conversation

valeriyvan
Copy link
Contributor

@valeriyvan valeriyvan commented Feb 1, 2023

Fix settings for custom control sensitivities:

  • make text fields for sensitivities editable only in custom mode
  • store values in settings on end of editing or on scene dismissal

This fixes crash - issue #25.

Despite the fix makes sense for me, settings screen seems to be or partially broken or not well thought:

  • I don't see even attempt to persist custom settings in the code. Is it a bug? Or design decision?
  • It should be a button to cancel all changes and return to values as they were when settings screen was open.

@valeriyvan
Copy link
Contributor Author

I would, probably, contribute much more to this repo provided PR are reviewed a little bit faster.

@ataffanel ataffanel merged commit 62cc9fd into bitcraze:master Feb 7, 2023
@ataffanel
Copy link
Member

Thanks! This is merged.

The settings screen could indeed be greatly improved, it was done a long time ago by an iOS dev. beginner (me).

Sorry for the long review time, I was preparing and travelling to FOSDEM and since we are not Mac user I have to have access to the office dev-mac to review iOS PRs. I think it would help greatly to have some CI in github actions. I will look at the possibility of setting that up. If you have any experience in setting up iOS build in github actions it would be great to have pointers.

@valeriyvan
Copy link
Contributor Author

https://github.com/marketplace/actions/ios-build-action

@valeriyvan
Copy link
Contributor Author

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