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

fix(a380x/mfd): PERF APPR minimums input logic #9666

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Pruznak
Copy link

@Pruznak Pruznak commented Dec 16, 2024

Fixes #9114

Summary of Changes

  1. RADIO field now accepts some text inputs - NO, NONE, NODH, NO DH
  2. Both BARO and RADIO fields do not accept values below 1 anymore
  3. Both BARO and RADIO fields cannot be filled out at the same time anymore - when one is filled and user tries to fill in the other one, it doesn't get filled, still displays placeholders instead.

Screenshots (if necessary)

References

Screenshot 2024-12-16 163148
Screenshot 2024-12-16 163126

Issue linked

Additional context

Discord username (if different from GitHub): Pruznak

Testing instructions

  1. Load up aircraft, initialize a flight plan and select any precision approach
  2. Navigate to PERF APPR page and try to enter the above listed strings (NONE, NO, NODH, NO DH) into the RADIO field and make sure they are accepted as a valid input.
  3. Try to enter any other string into both fields (+ the above mentioned strings into the BARO field) and make sure FORMAT ERROR is displayed in the "scratchpad"
  4. Try to enter 0 as decision height into both fields and make sure ENTRY OUT OF RANGE is displayed in the "scratchpad"
  5. Try to fill out both fields without clearing the other first - the field that you are trying to fill should just display ----- FT upon pressing enter. Delete the value in the filled in field and try to fill in the other one with a valid input - should go through normally.
  6. Enter any valid input into the RADIO field, navigate to any other MFD page and back to PERF APPR, make sure field is still filled in with the previously entered value.

How to download the PR for QA

Every new commit to this PR will cause new A32NX and A380X artifacts to be created, built, and uploaded.

  1. Make sure you are signed in to GitHub
  2. Click on the Checks tab on the PR
  3. On the left side, find and click on the PR Build tab
  4. Click on either flybywire-aircraft-a320-neo, flybywire-aircraft-a380-842 (4K) or flybywire-aircraft-a380-842 (8K) download link at the bottom of the page

return null;
}
if (input === 'NONE' || input === 'NO' || input === 'NO DH' || input === 'NODH') {
return input;
Copy link
Member

@tracernz tracernz Dec 17, 2024

Choose a reason for hiding this comment

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

It seems weird to not normalise them in the parser. Why not do that and avoid any other code having to deal with multiple different values that have the same meaning?

Copy link
Member

@Benjozork Benjozork Dec 17, 2024

Choose a reason for hiding this comment

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

I was gonna comment the same but assumed it needed to keep displaying the same text.

Copy link
Author

Choose a reason for hiding this comment

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

I also wanted to do that and return null, but when I did so, I came across a problem - when entering a value into the RADIO field, going to any other MFD page and then back to PERF APPR, the value go deleted, both from the field and the actual minimum variable. That's because InputField.ts sets the actual text value of the field from the specified variable of the component. The value of those strings in minimums altitude is basically null, so when rendering the page again, it would fall to the if which populates placeholders in the field, because the variable would be null. That's why it's done like this, to keep even the string value in the input field. If there's a better way to do this, I'm of course open to suggestions, it's just that I didn't figure out a better way to work with this.

Copy link
Author

Choose a reason for hiding this comment

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

It seems weird to not normalise them in the parser. Why not do that and avoid any other code having to deal with multiple different values that have the same meaning?

So what's the verdict on this one ?

@Pruznak
Copy link
Author

Pruznak commented Jan 9, 2025

@tracernz So is there anything I should change here ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🟣 QA Review: Ready to Test
Development

Successfully merging this pull request may close these issues.

[A380X] - APPR PERF decision height boxes - wrong input logic
4 participants