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: custom air conditioning #899

Merged
merged 8 commits into from
Oct 30, 2023
Merged

feat: custom air conditioning #899

merged 8 commits into from
Oct 30, 2023

Conversation

mjuhe
Copy link
Contributor

@mjuhe mjuhe commented Oct 7, 2023

Summary

This PR adds a page under Feature Guides to define the "Custom Air Conditioning System", mainly needed to support PR flybywiresim/aircraft#8178 [Air Conditioning Failures].

My first contribution to docs so open to all comments and suggestions for improvement 😄. I'm not good at this kind of thing.

Location

docs/fbw-a32nx/feature-guides/custom-air-conditioning/

Discord username (if different from GitHub): mjuhe

@vercel
Copy link

vercel bot commented Oct 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 29, 2023 4:10am

@mjuhe mjuhe changed the title Air conditioning feat: custom air conditioning Oct 7, 2023
@Valastiri
Copy link
Member

Valastiri commented Oct 12, 2023

Self note - Discuss options for a shorter title (for only the left navigation menu)

@mjuhe mjuhe marked this pull request as ready for review October 14, 2023 19:26
@github-actions github-actions bot added the Review Required PR Check Label label Oct 14, 2023
Copy link
Member

@Valastiri Valastiri left a comment

Choose a reason for hiding this comment

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

Woot thanks for the great PR! (forgot to add my intro) before sending the review. Have a couple suggestions and let me know if you have any questions!

Couple things TODO

  • I can optimize images for you as 400kb is rather large. Will get on this sometime today.
  • Is there any resolution method to the failures you've described? (beyond turning the failure off).

I would hazard the following (in relation to resolutions):

  • If yes -> maybe include basic resolution steps in an admonition or just via a new section
  • If no -> maybe include an admonition to simply state we will be bringing resolution methods or etc. at a later time

docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
@mjuhe
Copy link
Contributor Author

mjuhe commented Oct 16, 2023

Thanks for the review @Valastiri !! I've implemented your comments and added some extra lines to break up paragraphs. I still think it looks like a lot of text and some graphs/images might help, but I don't have any ideas on what else to add 😄

Regarding your comment about the failures resolution methods. There is no way to "fix" a failed item, but I've added a comment that says Make sure you follow the ECAM actions to mitigate each failure!. What do you think?

@Valastiri
Copy link
Member

Thanks for the review @Valastiri !! I've implemented your comments and added some extra lines to break up paragraphs. I still think it looks like a lot of text and some graphs/images might help, but I don't have any ideas on what else to add 😄

Regarding your comment about the failures resolution methods. There is no way to "fix" a failed item, but I've added a comment that says Make sure you follow the ECAM actions to mitigate each failure!. What do you think?

heyo! I'll take a look tonight sorry was quite busy past couple of days. Yea that works! I wasn't entirely sure if there were should've given ya that option. That ECAM migitations sounds great!

If you don't mind me committing things I'll maybe break up the text a bit and even if it's long we can iterate down the line with more media if you'd like. or we can wait.

@Valastiri Valastiri requested a review from Sleinmaster October 17, 2023 11:50
Copy link
Collaborator

@Sleinmaster Sleinmaster left a comment

Choose a reason for hiding this comment

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

Some spell-checking, will have a more in-depth read some other time when the duties allow it.

# Custom Air Conditioning System

## System Description
The Air Conditioning, Pressurization and Ventilation system (also known as Environmental Control System (ECS) or simply Air Conditioning System for convenience) regulates the temperature, pressure and airflow inside the aircraft.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The Air Conditioning, Pressurization and Ventilation system (also known as Environmental Control System (ECS) or simply Air Conditioning System for convenience) regulates the temperature, pressure and airflow inside the aircraft.
The air Conditioning, pressurization and ventilation system (also known as Environmental Control System (ECS) or simply Air Conditioning System for convenience) regulates the temperature, pressure, and airflow inside the aircraft.

Copy link
Member

Choose a reason for hiding this comment

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

@Sleinmaster question -- you missed the capital C for the first conditioning (intentional)?

Additionally, keeping the capitilization in the second instance yes?

Copy link
Member

Choose a reason for hiding this comment

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

went with "conditioning"

docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
docs/fbw-a32nx/feature-guides/custom-air-conditioning.md Outdated Show resolved Hide resolved
@Valastiri
Copy link
Member

I might review this tonight and commit changes to get this ready for merge

@Valastiri
Copy link
Member

check build

@github-actions
Copy link

Strict Build Failed! ❌ - Please checkout this PR locally to fix the issue before merging or visit the actions tab.

@Valastiri
Copy link
Member

will solve with rebase shortly with optimized images + some additional changes

@github-actions github-actions bot removed the Review Required PR Check Label label Oct 30, 2023
@github-actions github-actions bot merged commit cd2831e into primary Oct 30, 2023
5 of 6 checks passed
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.

3 participants