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

feat: React Chrono, BAT indicator, rudder trim indicator #3794

Merged
merged 11 commits into from
Mar 18, 2021

Conversation

lukecologne
Copy link
Member

@lukecologne lukecologne commented Mar 3, 2021

Fixes #3237
Fixes #3238
Replaces #3469

Summary of Changes

  • Reimplement BAT indicator in react
  • Reimplement RTPI in react
  • Reimplement Chrono in react
    • Stopwatch
    • Clock/Date - Note: switching from GPS time to internal time, and changing the internal time is not implemented yet.
    • Elapsed Time
      • Code
      • XML - Note: the elapsed time knob should be springloaded in the 3rd position (reset). This is not yet implemented. If someone has an idea of how to do this, feel free to make a suggestion.
  • The visuals should be pretty much identical.

Screenshots (if necessary)

References

Additional context

Discord username (if different from GitHub):

Testing instructions

  • Make sure the chrono and RTPI behave according to the references provided. (Except the issue described above)
  • Make sure the BAT indicator behaves properly.

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

@lukecologne lukecologne marked this pull request as ready for review March 4, 2021 00:11
@lukecologne
Copy link
Member Author

Should be ready for review. It still has one issue: the elapsed time knob should be springloaded in the 3rd position (reset). This is not yet implemented. If someone has an idea of how to do this, feel free to make a suggestion.

@beheh
Copy link
Member

beheh commented Mar 4, 2021

#3469 has landed, I only saw this afterwards. Could you please make sure to incorporate any changes from it, if you haven't already? Thanks!

Edit: Also there's #3448 - is that incorporated already too?

@lukecologne
Copy link
Member Author

lukecologne commented Mar 4, 2021

#3469 has landed, I only saw this afterwards. Could you please make sure to incorporate any changes from it, if you haven't already? Thanks!

Edit: Also there's #3448 - is that incorporated already too?

I just reimplemented the Chrono after the real life refernce, and that includes the change from this PR. Not sure what #3448 is supposed to do, the screenshot isn't really informative. I will resolve the conflicts from #3469

@lukecologne
Copy link
Member Author

Ok, conflicts are resolved.

@Benjozork Benjozork added this to the v0.6.0 milestone Mar 6, 2021
@lukecologne lukecologne force-pushed the react-chrono-bat branch 2 times, most recently from 3e9d795 to 331b2a0 Compare March 7, 2021 21:10
@beheh
Copy link
Member

beheh commented Mar 8, 2021

#3448 is about some CSS changes - maybe @Snapmatics could chime in or clarify what changes that are? Otherwise #3448 might have to be completely redone after this lands.

@lukecologne
Copy link
Member Author

lukecologne commented Mar 8, 2021

The only change that is not either whitespace or formatting is this: https://github.com/flybywiresim/a32nx/pull/3448/files?diff=split&w=1#diff-6117fb7e0993a922fb071577fcfd106edc6b554a8de8f97cb7f7237bd2a48257L70. I added a reference for the RTPI to my PR comment, the testers should compare it to my PR and I can adjust it if something looks off. Just copying the changes from #3448 is not going to work anyways, it uses %, and I use px here.

@lukecologne lukecologne force-pushed the react-chrono-bat branch 5 times, most recently from 94b1f64 to 1c7601f Compare March 15, 2021 19:55
@wpine215
Copy link
Member

is this ready for QA testing?

@lukecologne
Copy link
Member Author

From me, yes. Obviously someone with react knowledge needs to review first.

@beheh beheh self-requested a review March 16, 2021 00:40
@marcman86
Copy link
Contributor

Category: QA Tester
Name: marcman86#4907
Date of testing: 17.3.2021
Version of the sim: 1.14.5.0
PR Tested: #3794
Tier of testing: 1
Changes to observe: BAT, RTPI and Chrono
Testing technique: Tryout

Steps made to test the Pull Request:

1.) Spawned C&D at EDDP
2.) Looked at Bat displays
3.) Tested Chrono (Image1+2)
4.) Tested RTPT

Media:

136
Image1

137
Image2

Issues: Last two digits from timer and ET jump from colon and are cut off

Overall Rating: Needs to be improved

Conclusions of the testing: One issue, rest works fine. Chrono and RTPI displays are a little dim in daylight in my opinion.

Hope this helps :-)

@lukecologne lukecologne added Testing Started Testing for this PR has started and tweaking / fixing is underway and removed Ready to Test labels Mar 17, 2021
@marcman86
Copy link
Contributor

marcman86 commented Mar 17, 2021

Category: QA Tester
Name: marcman86#4907
Date of testing: 17.3.2021
Version of the sim: 1.14.5.0
PR Tested: #3794
Tier of testing: 1
Changes to observe: BAT, RTPI and Chrono
Testing technique: Tryout

Retested after fix: Looks good now

138

Issues: -

Overall Rating: Good

Conclusions of the testing: Good to go

Hope this helps :-)

@derl30n derl30n added the Testing Started Testing for this PR has started and tweaking / fixing is underway label Mar 17, 2021
@derl30n
Copy link
Contributor

derl30n commented Mar 17, 2021

sry mb, one more test missing.

@lukecologne
Copy link
Member Author

Issues (If Any): Write any issues
Shouldn't the letter "r" be in uppercase?
Probably not relative on this PR, shouldn't reset button decrease to 0.0 on RTPI

Thank you for the review. The problem with the lowercase R is in the font, nothing I can do there, I already use an uppercase R in the code. The rudder trim reset is unrelated, this PR only changed the display, nothing on the actual knob/button

@Matse08
Copy link

Matse08 commented Mar 18, 2021

Category: QA Tester
Name: Matse#7454
Date of testing: 18.03.21
Version of the sim: 1.14.5.0
PR Tested: #3794
Tier of testing: 2
Changes to observe:
BAT
RTPI
Chrono

Testing technique: Try-out
Steps you've made to test the Pull Request:

  1. Spawned cold and dark
  2. Tested RTPI
  3. Tested Chrono
  4. Tested BAT

Flight notes (When done a full flight):N/A

Issues (If Any): No issue

Overall Rating: Good
Conclusions of the testing: This does work great for me

@derl30n derl30n added Tested and removed Testing Started Testing for this PR has started and tweaking / fixing is underway labels Mar 18, 2021
@derl30n derl30n merged commit 8edf604 into flybywiresim:master Mar 18, 2021
@lukecologne lukecologne deleted the react-chrono-bat branch March 18, 2021 20:08
@derl30n
Copy link
Contributor

derl30n commented Mar 18, 2021

you are 1 second to late -.-

lukecologne added a commit to lukecologne/a32nx that referenced this pull request Mar 18, 2021
lukecologne added a commit that referenced this pull request Mar 18, 2021
Co-authored-by: DerL30N <leon.beeser@yahoo.de>
aguther pushed a commit that referenced this pull request Mar 19, 2021
Co-authored-by: DerL30N <leon.beeser@yahoo.de>
Sequal32 added a commit to Sequal32/yourcontrols that referenced this pull request Mar 21, 2021
Sequal32 added a commit to Sequal32/yourcontrols that referenced this pull request Apr 2, 2021
TopTrenDev added a commit to TopTrenDev/yourcontrols that referenced this pull request Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants