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

Small patch #2621

Merged
merged 1 commit into from Oct 6, 2021
Merged

Small patch #2621

merged 1 commit into from Oct 6, 2021

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Oct 4, 2021

Fixes selector (which also wrongly did target Rates Roll Center and Expo) and refactored condition variable to something meaningful as requested in previous PR.

@haslinghuis haslinghuis added this to the 10.8.0 milestone Oct 4, 2021
@haslinghuis haslinghuis self-assigned this Oct 4, 2021
@haslinghuis haslinghuis force-pushed the slider_patch branch 2 times, most recently from 10f2074 to 5dfee14 Compare October 5, 2021 02:07
@ctzsnooze
Copy link
Member

Changing the background colour of the PID numbers may have been a part of 4.2, but I think it is a bad idea.
It makes for very poor legibility in some situations...
Screen Shot 2021-10-05 at 13 42 51
Let's keep the numeric background the same; we have the warning box, and the colour behind the slider itself, that's enough.
Or at least, if we want to put them back, please make a separate PR to discuss that.
This PR should focus on bug-fixes arising from user feedback.

@haslinghuis haslinghuis force-pushed the slider_patch branch 3 times, most recently from cba74ec to 147704a Compare October 5, 2021 10:53
@sonarcloud
Copy link

sonarcloud bot commented Oct 5, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ctzsnooze ctzsnooze self-requested a review October 5, 2021 12:17
@ctzsnooze
Copy link
Member

Checked, tested.

@blckmn
Copy link
Member

blckmn commented Oct 5, 2021

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@haslinghuis haslinghuis merged commit 3f74761 into betaflight:master Oct 6, 2021
@haslinghuis haslinghuis deleted the slider_patch branch October 6, 2021 02:34
@yngndrw
Copy link
Contributor

yngndrw commented Jan 6, 2022

@ctzsnooze

Changing the background colour of the PID numbers may have been a part of 4.2, but I think it is a bad idea. It makes for very poor legibility in some situations... Screen Shot 2021-10-05 at 13 42 51 Let's keep the numeric background the same; we have the warning box, and the colour behind the slider itself, that's enough. Or at least, if we want to put them back, please make a separate PR to discuss that. This PR should focus on bug-fixes arising from user feedback.

Only just seen this comment, but the slider heatmaps were added in this PR:
#1839
... and changes for dark mode were added in this PR:
#1843

The text used to be black which made the numbers far more legible. The heatmaps were added so that you could see what changes each slider were making at a glance, the colouring of the sliders themselves are not equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants