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: show preselected speed and mach accordingly #8230

Closed
wants to merge 3 commits into from

Conversation

aaronschweig
Copy link

Fixes #8217

Summary of Changes

I added the colons to the SPEED SEL and MACH SEL messages in the FMA.tsx

Screenshots (if necessary)

References

Shown in the original issue #8217

Additional context

This is my first PR in this Repo - if I missed something or did something wrong please let me know - happy to learn!

Discord username (if different from GitHub):

Testing instructions

How to download the PR for QA

Every new commit to this PR will cause a new A32NX artifact 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, click on the bottom PR tab
  4. Click on the A32NX download link at the bottom of the page

@becas22
Copy link
Contributor

becas22 commented Sep 25, 2023

@aaronschweig it's missing the MACH SEL edit in line 634 of FMA.tsx, no?

@aaronschweig
Copy link
Author

@becas22 of course it does - stupid me even asked in the issue but forgot to commit it. Sorry for that 😄

@BlueberryKing
Copy link
Member

Hi, thank you for the PR! Can you add a changelog entry? Also, I'm a bit unsure if there is a space after the colon, but I'd have to compare how it looks in the sim vs. the references.

@tracernz
Copy link
Member

tracernz commented Sep 26, 2023

Should have a changelog entry, and ideally a screenshot for all graphical changes.

-e- Also, is the colon the right size? It looks smaller than the surrounding text in the photograph on the linked issue.

@aaronschweig
Copy link
Author

aaronschweig commented Sep 27, 2023

Hey sorry for the long delay, real life came in!

I took some screenshots from in the sim:

(Active pause just kept increasing the speed 😋 )

SPEED SEL:

SPEED SEL

MACHSEL:

MACH SEL

After comparing it with the example in the original issue it looks like the space should go away, but I could also be mistaken due to the angle of the screenshot in the original issue. But also wrt the reference manual provided by @becas22 it looks like there should not be a space. What do you think?

@becas22
Copy link
Contributor

becas22 commented Sep 27, 2023

@aaronschweig yeah, looks like the space needs to go. Can you provide some screenshots without the space?

@tshomas
Copy link
Contributor

tshomas commented Sep 27, 2023

Got this one today from work in case you still need refs!

PXL_20230927_064133817

Tried to get a speed sel but happened too quickly

@aaronschweig
Copy link
Author

Here is a screenshot without the space:

image

@becas22
Copy link
Contributor

becas22 commented Sep 27, 2023

From Shomas pic it seems that:

  • the ":" are aligned at the bottom of the line;
  • There seems to be some space but not a whole one.

@aaronschweig do you think these are possible to implement? Otherwise I would say that the one without the space looks the best

@aaronschweig
Copy link
Author

I played around a bit, but making it smaller or try fiddleing with the whitespace would involve messing with the text node of the svg in one way or the other. I couldn't figure out a way that looks right and works with a reasonable amount of changes in the code.

Also one question to the changelos: Is it just written by myself or autogenerated?

@becas22
Copy link
Contributor

becas22 commented Sep 29, 2023

Then I would say that it looks better now (without the space) than what it was before (with no : at all)

I would say that the changelog needs to be done by you. Take a look at this PR by BBKing for example: be80aa9

Yours should be here: https://github.com/aaronschweig/aircraft/blob/master/.github/CHANGELOG.md

@tshomas
Copy link
Contributor

tshomas commented Sep 30, 2023

PXL_20230930_192526282

Another one, not the clearest unfortunately though!

@BravoMike99
Copy link
Contributor

Hi @aaronschweig do you plan on working this further? I wouldn't mind taking a look at this issue in case you don't plan to.

@aaronschweig
Copy link
Author

Hey @BravoMike99 yes please feel free to continue and make changes!

@BravoMike99
Copy link
Contributor

@aaronschweig just a FYI, I created #8328 as a continuation of this PR and gave you credits on the changelog. I guess in the meantime this PR can be closed.

@Maximilian-Reuter
Copy link
Member

Closed as continuation is happening in different PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ❌ Cancelled/Unfinished
Development

Successfully merging this pull request may close these issues.

PFD SPEED SEL message
8 participants