Adding dark and light theme names to the sponsor request (params to API) #3561

blckmn · 2023-09-02T15:34:30Z

I couldn't see any way to identify whether or not the dark theme was enabled or not. So added the property to DarkTheme.

If anyway has a better idea please let me know.

haslinghuis · 2023-09-02T15:58:42Z

We have something like:

betaflight-configurator/src/js/tabs/receiver.js

Lines 563 to 565 in ff4b62d

    
           DarkTheme.isDarkThemeEnabled(function(isEnabled) { 
        
               windowWatcherUtil.passValue(createdWindow, 'darkTheme', isEnabled); 
        
           });

and

betaflight-configurator/src/js/tabs/options.js

Lines 180 to 189 in ff4b62d

    
           options.initDarkTheme = function () { 
        
               $('#darkThemeSelect') 
        
                   .val(DarkTheme.configEnabled) 
        
                   .change(function () { 
        
                       const value = parseInt($(this).val()); 
        
                       setConfig({'darkTheme': value}); 
        
                       setDarkTheme(value); 
        
                   }).change(); 
        
           };

asizon · 2023-09-02T16:13:03Z

We have something like:

betaflight-configurator/src/js/tabs/receiver.js

Lines 563 to 565 in ff4b62d

DarkTheme.isDarkThemeEnabled(function(isEnabled) {

windowWatcherUtil.passValue(createdWindow, 'darkTheme', isEnabled);

});

and

betaflight-configurator/src/js/tabs/options.js

Lines 180 to 189 in ff4b62d

options.initDarkTheme = function () {

$('#darkThemeSelect')

.val(DarkTheme.configEnabled)

.change(function () {

const value = parseInt($(this).val());

setConfig({'darkTheme': value});

setDarkTheme(value);

}).change();

};

Yes,i think is better to go with the current available function.

blckmn · 2023-09-02T17:17:04Z

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 -> FAIL
Tested label found -> FAIL
assigned to an approver -> PASS
approver count at least three -> FAIL

haslinghuis · 2023-09-02T18:06:52Z

src/js/BuildApi.js

-    loadSponsorTile(onSuccess, onFailure) {
-        const url = `${this._url}/api/configurator/sponsors`;
+    loadSponsorTile(mode, onSuccess, onFailure) {
+        const url = `${this._url}/api/configurator/sponsors/${mode}`;


Checking for window.matchMedia('(prefers-color-scheme: dark)').matches would do

I'll test this.

haslinghuis · 2023-09-02T18:07:30Z

src/js/DarkTheme.js


 const DarkTheme = {
    configEnabled: undefined,
+    enabled: false,


This would duplicate configEnabled

Actually I'm not sure it does. It looked to me that configEnabled is not binary, so supports the option 'honour the system' so can't be relied upon for stating that dark theme was in fact enabled.

Above statement would extract the value or [use] get ConfigStorage darkTheme value.

Which means configEnabled is not well named. It should be configSetting or similar. It can't be relied upon as to if it's actually applied or not... sure it'll have a value of two... but does that mean dark theme is active? I don't think it does because the test of isEnabled is not persisted...

… or not the darkTheme is currently enabled (just its setting).

sonarqubecloud · 2023-09-02T18:43:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
1.7% Duplication

blckmn · 2023-09-02T18:50:04Z

We have something like:

betaflight-configurator/src/js/tabs/receiver.js

Lines 563 to 565 in ff4b62d

DarkTheme.isDarkThemeEnabled(function(isEnabled) {

windowWatcherUtil.passValue(createdWindow, 'darkTheme', isEnabled);

});

betaflight-configurator/src/js/DarkTheme.js

Lines 44 to 46 in 9599c0a

    
           if (chrome.app.window !== undefined) { 
        
               windowWatcherUtil.passValue(chrome.app.window.get("receiver_msp"), 'darkTheme', isEnabled); 
        
           }

I actually don't like this approach. The reason is both receiver.js and darktheme.js are coupled together. It would be better that receiver (and any others) registers its interest (observes) the enabled / disabled aspect of darktheme (and gets a callback when it happens). Then darktheme is entirely standalone... and not coupled to any tabs.

github-actions · 2023-09-02T18:53:16Z

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 · 2023-09-02T18:59:01Z

Agree, it's part of code from 2020 to fix dark theme on Android.

blckmn · 2023-09-02T19:03:10Z

Agree, it's part of code from 2020 to fix dark theme on Android.

I'll clean it up.

blckmn · 2023-09-27T04:41:37Z

@haslinghuis @nerdCopter this is good to go as is... I will fix up the call backs separately in another PR

…PI) (betaflight#3561) * Adding dark and light theme names to the sponsor request (params to API) * Renamed configEnabled to configSetting as it does not reflect whether or not the darkTheme is currently enabled (just its setting). * Missed the configEnabled in the optionsTab.

Adding dark and light theme names to the sponsor request (params to API)

db3e958

This comment has been minimized.

Sign in to view

haslinghuis reviewed Sep 2, 2023

View reviewed changes

blckmn added 2 commits September 3, 2023 04:40

Renamed configEnabled to configSetting as it does not reflect whether…

686ef30

… or not the darkTheme is currently enabled (just its setting).

Missed the configEnabled in the optionsTab.

9599c0a

blckmn requested review from haslinghuis and nerdCopter September 27, 2023 04:40

blckmn self-assigned this Sep 27, 2023

blckmn added the Ready to Merge label Sep 27, 2023

blckmn added this to the 10.10.0 milestone Sep 27, 2023

haslinghuis approved these changes Sep 27, 2023

View reviewed changes

nerdCopter approved these changes Sep 27, 2023

View reviewed changes

haslinghuis merged commit e120c48 into betaflight:master Sep 27, 2023

blckmn deleted the sponsors branch September 27, 2023 20:42

Uh oh!

Adding dark and light theme names to the sponsor request (params to API) #3561

Adding dark and light theme names to the sponsor request (params to API) #3561

Uh oh!

Conversation

blckmn commented Sep 2, 2023

Uh oh!

This comment has been minimized.

haslinghuis commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asizon commented Sep 2, 2023

Uh oh!

blckmn commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haslinghuis Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

blckmn Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

haslinghuis Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

blckmn Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

haslinghuis Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blckmn Sep 2, 2023

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 2, 2023

Uh oh!

blckmn commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 2, 2023

Uh oh!

haslinghuis commented Sep 2, 2023

Uh oh!

blckmn commented Sep 2, 2023

Uh oh!

blckmn commented Sep 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

haslinghuis commented Sep 2, 2023 •

edited

Loading

blckmn commented Sep 2, 2023 •

edited

Loading

haslinghuis Sep 2, 2023 •

edited

Loading

blckmn commented Sep 2, 2023 •

edited

Loading