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

Color for vtx ready status #3422

Merged
merged 7 commits into from Apr 27, 2023
Merged

Conversation

HThuren
Copy link
Member

@HThuren HThuren commented Apr 15, 2023

Change to Device ready has green / red color, are moved up in presentation and are updated if device status change.
image

@github-actions

This comment has been minimized.

@HThuren HThuren changed the title Color vt xstatus Color for vtx ready status Apr 15, 2023
@blckmn
Copy link
Member

blckmn commented Apr 15, 2023

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 -> FAIL
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

It's cleaner for the translators simply apply the class to the element depending on the content. It's how we it usually in the rest of the code and does not imply new messages with class included.

@HThuren
Copy link
Member Author

HThuren commented Apr 15, 2023

It's cleaner for the translators simply apply the class to the element depending on the content. It's how we it usually in the rest of the code and does not imply new messages with class included.

I am not sure what you address, I used the same setup as GPS fixed, can you give an exampel ?
This one

vtxReadyFalse = "<span class="fixfalse">No"

to embed the class in js or html ?

@haslinghuis haslinghuis added this to the 10.10.0 milestone Apr 15, 2023
@HThuren HThuren requested a review from McGiverGim April 15, 2023 22:42
@github-actions

This comment has been minimized.

@McGiverGim
Copy link
Member

Sorry for my late response, but I was out. What I was talking about is to do something like this:

$(".armedicon").toggleClass('active', bit_check(FC.CONFIG.mode, i));

More info here: http://api.jquery.com/toggleclass/

This will add/remove the class depending on the second parameter true/false. In this way the messages file is clean, only with basic 'true' and 'false', and we use this in the same part of the code where we decide what to show, and where we assign MSP values to fields.

In the CSS file you can have a "red" value for the 'basic' class, and a gree value when the 'basic' class has a 'valid' class too, something similar to (the names are only for reference, better to use something more significative):

.basic {
  color: red;
}
.basic .valid {
  color: green;
}

Add a "custom" method like your latest version, I think is not the best way, because maybe we want it for YES/NO, or TRUE/FALSE, or ACEPTED/REJECTED, or VALID/NOT VALID... etc.

@HThuren
Copy link
Member Author

HThuren commented Apr 18, 2023

@McGiverGim , no worry, I also have a fulltime job :-)

Very smart and rigorous way you decribe, I go for it .-.

@HThuren
Copy link
Member Author

HThuren commented Apr 22, 2023

hi @McGiverGim

Well, I havn't suceed in the requested change, don't you think we still have a 'clean' message file with this construction

export function getColorYesNo(value) {
    return (value ? `<span class="fixtrue">${i18n.getMessage("Yes")}</span>` : `<span class="fixfalse">${i18n.getMessage("No")}</span>`);
}

called fx
$('.GPS_info td.fix').html(getColorYesNo(FC.GPS_DATA.fix));

@Benky
Copy link
Contributor

Benky commented Apr 24, 2023

Could it be something like this: Benky@aeaf2fd ?

@HThuren
Copy link
Member Author

HThuren commented Apr 24, 2023

Could it be something like this: Benky@aeaf2fd ?

So simple and elegant :-) - thats the way

@haslinghuis
Copy link
Member

Also notice plugging LIPO updates status - but removing LIPO the status remains. But is outside the scope of this PR.

@HThuren
Copy link
Member Author

HThuren commented Apr 25, 2023

Also notice plugging LIPO updates status - but removing LIPO the status remains. But is outside the scope of this PR.

yes, I'm aware, another PR. :-)

@github-actions

This comment has been minimized.

src/js/tabs/gps.js Outdated Show resolved Hide resolved
src/js/tabs/gps.js Outdated Show resolved Hide resolved
src/js/tabs/setup.js Outdated Show resolved Hide resolved
src/js/tabs/setup.js Outdated Show resolved Hide resolved
src/js/tabs/vtx.js Outdated Show resolved Hide resolved
src/js/tabs/vtx.js Outdated Show resolved Hide resolved
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.

One left :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@haslinghuis
Copy link
Member

@McGiverGim should be good to go now

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

A lot more functionality very interesting. The code seems ok to me.
But the less/css code can be a lot better using the less possibilities. Only one difference between the active and the basis class.
I'm on mobile, tomorrow I can suggest the exact code. Sorry but my real work has all my time lately.

@HThuren
Copy link
Member Author

HThuren commented Apr 25, 2023

A lot more functionality very interesting. The code seems ok to me. But the less/css code can be a lot better using the less possibilities. Only one difference between the active and the basis class. I'm on mobile, tomorrow I can suggest the exact code. Sorry but my real work has all my time lately.

No worry and hurry, I look forward to learn about CSS/less

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Hi! Sorry for my late response, but I think this will work.

src/css/main.less Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@haslinghuis
Copy link
Member

@HThuren LGTM, only two unresolved issues remaining

@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2023

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!

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

For the rest to me is ok. Thanks for the changes.

@haslinghuis haslinghuis merged commit b523f0a into betaflight:master Apr 27, 2023
9 checks passed
@HThuren HThuren deleted the colorVTXstatus branch April 27, 2023 17:12
@haslinghuis
Copy link
Member

haslinghuis commented Aug 1, 2023

@HThuren there is a problem. After save it does not retrieve status anymore - going false (red). Please revisit.

EDIT: Fixed in #3538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

5 participants