-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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/nd): Update ND and VD Range Change Messages #9739
base: master
Are you sure you want to change the base?
fix(a380x/nd): Update ND and VD Range Change Messages #9739
Conversation
Could someone possibly assist with some guidance on the VD Range Change part of this PR? Currently the text is always visible even though I set a initial defaul value of 0, assuming that would be falsy, before I try to capture the ndRangeSetting change event. |
You probably need to use a boolean to drive the visibility of the message and whenever the range is changed, set that to true and after a timeout set it to false. You can take the logic of the message on the ND for some inspiration as it behaves like this. |
Ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! And apologies for the late review.
@@ -47,6 +48,8 @@ export class VerticalDisplayDummy extends DisplayComponent<VerticalDisplayProps> | |||
(r) => a380EfisRangeSettings[r], | |||
); | |||
|
|||
private readonly RangeChangeInProgress = ConsumerSubject.create(this.sub.on('set_range_change').whenChanged(), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private readonly RangeChangeInProgress = ConsumerSubject.create(this.sub.on('set_range_change').whenChanged(), false); | |
private readonly rangeChangeInProgress = ConsumerSubject.create(this.sub.on('set_range_change').whenChanged(), false); |
camelCase please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -189,6 +192,11 @@ export class VerticalDisplayDummy extends DisplayComponent<VerticalDisplayProps> | |||
FL | |||
</text> | |||
</g> | |||
<g visibility={this.RangeChangeInProgress.map((it) => (it ? 'visible' : 'hidden'))}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<g visibility={this.RangeChangeInProgress.map((it) => (it ? 'visible' : 'hidden'))}> | |
<g visibility={this.rangeChangeInProgress.map((it) => (it ? 'visible' : 'hidden'))}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -34,3 +34,8 @@ | |||
.a380xInvisible { | |||
display: none; | |||
} | |||
|
|||
.a380xModeRangeChange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid aircraft-type specific CSS class names. You can use e.g. .mode-range-change
, and define them differently in the respective aircraft's style.scss.
This means you would also have to add that class to the a32nx style.scss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use non-aircraft specific class, thanks!
Fixes #9473
Summary of Changes
Updated Range Change flag to use a prop and defined relevant values for both A32NX and A380X
Added CSS class for A380X as ND RANGE CHANGE message is different font size and position
Added VD Range Change message
Screenshots (if necessary)
A320 Range Change
A380 ND Range Change
A380 VD Range Change
A320 Mode Change
A380 Mode Change
References
Additional context
Discord username (if different from GitHub): MrJigs.
Testing instructions
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.