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

Move receiver and rssi configuration to receiver tab #2386

Merged
merged 1 commit into from
Feb 21, 2021

Conversation

haslinghuis
Copy link
Member

Fixes: #81

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.

First of all thanks for all this PRs that make the Configurator easier and more intuitive.
Apart from the gulpfile that you have detected...
Only some comments about the css.
What I see is that it has A LOT of new styles. Are all of them needed for that? Usually the major part of styles must be in the common css, only some differences if needed must be here, but I see a long list of them.
But the CSS is mesh of updates that maybe we can never clean so maybe this is the simple way to move this :)

@@ -140,6 +140,36 @@
background-color: #14407a;
}

/* from configuration tab */
Copy link
Member

Choose a reason for hiding this comment

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

Is better to remove the comment or change from something more "general" like "rx configuration". Any future user will not know nothing about what was in the configuration tab and what not and can confuse them.

line-height: 18px;
}

.tab-receiver .serialRXBox, .spiRxBox {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this must be:

Suggested change
.tab-receiver .serialRXBox, .spiRxBox {
.tab-receiver .serialRXBox, .tab-receiver .spiRxBox {

@@ -559,6 +589,10 @@
margin-bottom: 10px;
}

select.features.rxMode, select.serialRX {
Copy link
Member

Choose a reason for hiding this comment

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

This does not need the .tab-receiver to maintain consistence with the other styles? If it is a general feature style, maybe is better to go in a common css file. Is this if only for this tab, is better to add the .tab-receiver to go with the others styles.

Suggested change
select.features.rxMode, select.serialRX {
.tab-receiver select.features.rxMode, .tab-receiver select.serialRX {

.tab-receiver .gps td:nth-child(2) {
width: auto;
}
select.features.rxMode, select.serialRX, .dshotbeacon > td > div.select > div, .dshotbeacon select.dshotBeeperBeaconTone {
Copy link
Member

Choose a reason for hiding this comment

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

The same than before about adding the .tab-receiver to the style.

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 15, 2021

Fixed previous issues with another iteration (starting over, going backwards, hacking configuration to a minimum to see what's happening and really needed)

Have added a class receiver in html and css fixing some styling issues in this migration on line 584:
.tab-receiver .receiver select {

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 15, 2021

Okay. Because configuration does Save and Reboot and receiver tab only does Save, now it shows the Save and Reboot instead of Save button only when changing receiver mode, serial receiver provider or RSSI_ADC.

@sonarcloud
Copy link

sonarcloud bot commented Jan 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 1 Bug
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

@McGiverGim
Copy link
Member

Is this out of draft? Sometimes we don't review the code because we are waiting until some kind of "final form" is ready...

@haslinghuis
Copy link
Member Author

It's ready but it needs clean up , but I don't want conflict with #2356

@haslinghuis
Copy link
Member Author

haslinghuis commented Feb 17, 2021

Dark mode fix included 🕶️

@sonarcloud
Copy link

sonarcloud bot commented Feb 17, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mikeller
Copy link
Member

Looks good now.

Potentially one future improvement would be to move both ADC and RC channel based RSSI into the same panel, and only allow one to be selected at any time:

image

@mikeller mikeller added this to the 10.8.0 milestone Feb 21, 2021
@mikeller mikeller merged commit e06210a into betaflight:master Feb 21, 2021
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.

Rework 'Receiver' tab
3 participants