-
Notifications
You must be signed in to change notification settings - Fork 275
feat(ui): add always-visible ATX LED status indicators #1099
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
base: dev
Are you sure you want to change the base?
Conversation
- Add ATXStateIndicator component to ActionBar - Show Power/HDD LED status when ATX extension is active - Add popover with Power/Reset buttons for quick access - Add confirmation dialogs for power actions to prevent accidents Note: Long-press support for force power off can be added in a future iteration - the backend already supports "power-long" action. Fixes jetkvm#729
IDisposable
left a comment
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.
The UI needs a localization pass
| import { Button } from "@components/Button"; | ||
| import { ConfirmDialog } from "@components/ConfirmDialog"; | ||
| import { cx } from "@/cva.config"; | ||
| import { JsonRpcResponse, useJsonRpc } from "@/hooks/useJsonRpc"; |
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.
Should be from "@hooks/useJsonRpc"; (note the removed /)... and then sort up above the @components for lint happiness.
| const [showResetConfirm, setShowResetConfirm] = useState(false); | ||
| const [isActionInProgress, setIsActionInProgress] = useState(false); | ||
|
|
||
| const { send } = useJsonRpc(function onRequest(resp) { |
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.
resp is a placeholder for "response", but this is actually a request, so let's swap these to req please...
|
|
||
| // Check if ATX extension is active | ||
| useEffect(() => { | ||
| send("getActiveExtension", {}, (resp: JsonRpcResponse) => { |
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 is a response, so this is correct :)
| send("getATXState", {}, (resp: JsonRpcResponse) => { | ||
| if ("error" in resp) { | ||
| notifications.error( | ||
| `Failed to get ATX state: ${resp.error.data || "Unknown error"}`, |
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 needs to follow the normal localization patterns... see code like this
| setIsActionInProgress(false); | ||
| if ("error" in resp) { | ||
| notifications.error( | ||
| `Failed to send ATX power action: ${resp.error.data || "Unknown error"}`, |
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.
| }, [send]); | ||
|
|
||
| // Fetch ATX state when extension is active | ||
| useEffect(() => { |
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.
Doing this useEffect( will cause us to poll for the extension state here in the indicator and in the ATXPowerControl.tsx code... it should be plausible to share that state from stores.ts
| variant="warning" | ||
| confirmText="Power" | ||
| cancelText="Cancel" | ||
| onConfirm={() => handlePowerAction("power-short")} |
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.
Can we have two buttons here (long-press or short-press)?
Note: Long-press support for force power off can be added in a future iteration - the backend already supports "power-long" action.
Fixes #729