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

Add some missing style and minor bugfix #2996

Merged
merged 1 commit into from Oct 5, 2022

Conversation

ASDosjani
Copy link
Contributor

@ASDosjani ASDosjani commented Aug 15, 2022

I added the missing styles to the selectable lists, those white boxes in dark mode just hurt my eyes so I made this fix. The Larson scanner and the Blink always overlays didn't worked with each other properly so they toggle each other, only one of them can be used at a time. I noticed that the channel based color modifier doesn't have default value so I nominated the Throttle for this. Also updated deprecated .change() function.

While I was making this fix it stood out that the switch animations doesn't work if I select anyting on the Led table. It is not related to my code, I tried to fix it but I couldn't. If anybody has time I would appreciate a fix for this.
Edit: I could fix it.

@github-actions

This comment has been minimized.

@ASDosjani ASDosjani marked this pull request as draft August 15, 2022 14:54
@ASDosjani ASDosjani marked this pull request as ready for review August 15, 2022 15:01
@blckmn
Copy link
Member

blckmn commented Aug 15, 2022

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • 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 -> FAIL
  • approver count at least three -> FAIL

@KarateBrot
Copy link
Member

KarateBrot commented Sep 9, 2022

You can reopen this PR when you push something again

@ASDosjani ASDosjani changed the title Add missing style to the dropdown lists on LED strip tab Add missing style to the dropdown lists on LED strip tab and set throttle as default for aux selection list Sep 9, 2022
@ASDosjani ASDosjani reopened this Sep 9, 2022
@ASDosjani ASDosjani changed the title Add missing style to the dropdown lists on LED strip tab and set throttle as default for aux selection list Add some missing style and minor bugfix Sep 9, 2022
src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
@@ -1121,7 +1144,7 @@ led_strip.initialize = function (callback, scrollPosition) {
return mc.color;
}
}
return "";
return 3; //Throttle is the default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 is the index of the throttle in the list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be more specific, What list?

Copy link
Contributor Author

@ASDosjani ASDosjani Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
This one. There were no default value.

Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry missed that one in previous review

src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
@ASDosjani ASDosjani force-pushed the list-fix branch 2 times, most recently from 99bb4d6 to 95250b0 Compare September 9, 2022 20:23
haslinghuis
haslinghuis previously approved these changes Sep 9, 2022
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis moved this from Configurator to Ready to merge in Finalizing Firmware 4.4 Release Sep 18, 2022
blckmn
blckmn previously approved these changes Sep 19, 2022
src/js/tabs/led_strip.js Outdated Show resolved Hide resolved
@ASDosjani ASDosjani dismissed stale reviews from blckmn and haslinghuis via 8efe30a September 19, 2022 09:38
@ASDosjani ASDosjani requested review from blckmn, haslinghuis and asizon and removed request for asizon, blckmn and haslinghuis September 21, 2022 12:13
@haslinghuis haslinghuis moved this from Needs work to Ready to merge in Finalizing Firmware 4.4 Release Sep 21, 2022
@github-actions

This comment has been minimized.

Comment on lines 868 to 872
if (!areOverlaysActive(activeFunction)) $('.overlays').hide();
if (!areModifiersActive(activeFunction)) $('.modifiers').hide();
if (!areBlinkersActive(activeFunction)) $('.blinkers').hide();
if (!isWarningActive(activeFunction)) $('.warningOverlay').hide();
if (!isVtxActive(activeFunction)) $('.vtxOverlay').hide();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!areOverlaysActive(activeFunction)) $('.overlays').hide();
if (!areModifiersActive(activeFunction)) $('.modifiers').hide();
if (!areBlinkersActive(activeFunction)) $('.blinkers').hide();
if (!isWarningActive(activeFunction)) $('.warningOverlay').hide();
if (!isVtxActive(activeFunction)) $('.vtxOverlay').hide();
$('.overlays').toggle(areOverlaysActive(activeFunction);
$('.modifiers').toggle(areModifiersActive(activeFunction);
$('.blinkers').toggle(areBlinkersActive(activeFunction);
$('.warningOverlay').toggle(isWarningActive(activeFunction);
$('vtxOverlay').toggle(isVtxActive(activeFunction);

Also remove the 5 paragraphs below

Copy link
Contributor Author

@ASDosjani ASDosjani Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not working, I tested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, need to remove the negation ( ! ) (updated suggestion) and also need to remove the lines

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a better solution, please check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using toggle is a much better and dense solution,

Copy link
Contributor Author

@ASDosjani ASDosjani Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, it's working now.

@sonarcloud
Copy link

sonarcloud bot commented Sep 24, 2022

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

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis moved this from Ready to merge to Configurator in Finalizing Firmware 4.4 Release Oct 4, 2022
@SupaflyFPV
Copy link
Contributor

Mac works as expected...

@asizon asizon merged commit 600fa60 into betaflight:master Oct 5, 2022
Finalizing Firmware 4.4 Release automation moved this from Configurator to Closed Oct 5, 2022
@ASDosjani ASDosjani deleted the list-fix branch June 11, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants