Conversation
There was a problem hiding this comment.
Pull request overview
Makes the camera capture controls usable in the new profile pane design by ensuring the “Take Photo / Retake / Use Photo” actions are visibly presented and toggled through the capture flow.
Changes:
- Introduces a
setButtonVisible()helper to toggle button visibility viastyle.display. - Updates button labels and initial visibility for “Take Photo”, “Retake”, and “Use Photo”.
- Adjusts
getUserMediasuccess/failure handling to show the shutter button on preview start and to handle preview start failures.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| retakeButton.textContent = 'Retake' | ||
| setButtonVisible(retakeButton, false) |
There was a problem hiding this comment.
retakeButton is created via widgets.button(dom, retakeIcon, 'Retake'), which appends an <img> icon. Setting retakeButton.textContent = 'Retake' replaces all children and removes the icon/tooltip. If the intent is a text-only button, create it as a text button (e.g., call widgets.button without an icon) instead of overwriting textContent, or update the widget to render icon + label without destroying children.
| shutterButton.textContent = 'Take Photo' | ||
| setButtonVisible(shutterButton, false) |
There was a problem hiding this comment.
shutterButton is initially created with an icon (widgets.button(..., 'Snap')), but shutterButton.textContent = 'Take Photo' replaces the button contents and removes the icon/tooltip. If a labeled button is desired, prefer constructing it as a text button (no icon) or adjust the widget to support icon + label instead of overwriting textContent.
| sendButton.textContent = 'Use Photo' | ||
| setButtonVisible(sendButton, false) |
There was a problem hiding this comment.
sendButton comes from widgets.continueButton(dom), which creates an icon-only button. Setting sendButton.textContent = 'Use Photo' removes that icon and any tooltip set on it. If the goal is to switch to a text label, create a text button directly (or update the continue button/widget to support icon + label) rather than replacing textContent.
| shutterButton.style.visibility = 'visible' // Enable | ||
| sendButton.style.visibility = 'collapse' | ||
| retakeButton.style.visibility = 'collapse' | ||
| setButtonVisible(shutterButton, true) |
There was a problem hiding this comment.
There is inconsistent indentation on the setButtonVisible(shutterButton, true) line inside the getUserMedia(...).then(...) block, which makes the block harder to read and may trip linting/formatting checks. Align it with the other statements in the block.
| setButtonVisible(shutterButton, true) | |
| setButtonVisible(shutterButton, true) |
| const setButtonVisible = (button: HTMLElement, visible: boolean) => { | ||
| button.style.display = visible ? 'inline-flex' : 'none' | ||
| } |
There was a problem hiding this comment.
New UI-state logic is introduced via setButtonVisible(...)/style.display and the updated state transitions in displayPlayer()/reviewImage(), but the existing unit test for this module only asserts the navigator.mediaDevices not available throw. Consider adding a Jest test that stubs navigator.mediaDevices.getUserMedia to resolve and then asserts the expected style.display values (and button labels) for shutter/retake/send in each state.
When trying to use for the new design of the profile pane I wasn't able to take a picture. I was only able to pause the video, there was a black button underneath, but that disappeared. I'm not sure how that was supposed to work exactly. Take Photo, Retake photo, and Use Photo are now visible and it works. I have only tested this when I copied the code over to profile pane and worked on it there. I've copied the changes exactly. Will test further.