-
-
Notifications
You must be signed in to change notification settings - Fork 17
Pause indicator from Help Wanted Issue #553 #702
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
Conversation
* @since 1.0.0 | ||
*/ | ||
export function useFlightStatus() { | ||
const [flight] = q.flight.active.useNetRequest(); |
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.
All of this information is pretty much already accessible from the q.flight.active
request. This hook is just an extra hoop to jump through. Let's keep it simple and remove this hook please.
FLIGHT PAUSED | ||
</span> | ||
<span className="text-lg text-center opacity-80 select-none"> | ||
Please wait for the flight director to resume the simulation |
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 can't count on a Flight Director being part of the mission. Let's just remove this copy. We don't need the extra text to explain what's going on.
aria-live="polite" | ||
> | ||
<div className="panel panel-warning px-12 py-8 flex flex-col items-center gap-6 shadow-2xl max-w-md mx-4"> | ||
<Icon name="ban" size="2xl" aria-hidden="true" className="text-warning" /> |
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.
I'd prefer to use an actual pause icon rather than the ban icon, which means something very different elsewhere in the UI. Something like this is good. https://lucide.dev/icons/octagon-pause
> | ||
<div className="panel panel-warning px-12 py-8 flex flex-col items-center gap-6 shadow-2xl max-w-md mx-4"> | ||
<Icon name="ban" size="2xl" aria-hidden="true" className="text-warning" /> | ||
<span className="font-bold text-4xl select-none text-center"> |
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.
I'd prefer to have the text be lowercase and then use CSS to style it as uppercase using the uppercase
Tailwind class. That renders better for some screen readers.
/binaries/ | ||
|
||
.wrangler | ||
.wrangler |
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.
I'd prefer if we didn't add extra ignores here. You can ignore the files for just your repo by following this suggestion: https://stackoverflow.com/a/653458/4697675
* @since 1.0.0 | ||
*/ | ||
export function FlightPauseIndicator() { | ||
const { isPaused } = useFlightStatus(); |
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.
Replace this with a q.flight.active
request and remove the extraneous hook.
import { Icon } from "@thorium/ui/Icon"; | ||
import { useFlightStatus } from "@thorium/hooks/useFlightStatus"; | ||
|
||
/** |
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.
This comment is unnecessarily verbose for a relatively small, self-explanatory component. Did an AI write this?
aria-live="polite" | ||
> | ||
<div className="panel panel-warning px-12 py-8 flex flex-col items-center gap-6 shadow-2xl max-w-md mx-4"> | ||
<Icon name="ban" size="2xl" aria-hidden="true" className="text-warning" /> |
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.
It also seems like size="2xl"
doesn't pass type checks properly for some reason.
Great feedback! I'm an old rusty dev and new to the game. I'll catch on soon. |
Closing due to lack of activity. If you want to address the feedback and get this over the finish line, we can reopen it. |
Summary
This PR adds a new pause indicator that displays prominently on station screens when a flight is paused. This
improves the user experience by providing clear visual feedback to crew members when the simulation is not
actively running.
Changes
Implementation Details
New Components:
Visual Design:
Testing
Screenshots
Breaking Changes
None - This is a purely additive feature that doesn't affect existing functionality.
PS. Also added some ignore rules for my dev setup. Should be harmless.
Closes #553