Skip to content

Conversation

@microbit-robert
Copy link

@microbit-robert microbit-robert commented Oct 31, 2024

  • Add 'done' state to recording dialog after recording(s) finished
  • Add additional recording options
  • Add userSelect: none to recording status under recording button
  • Use isRecording state to draw overlays on live graph

@github-actions
Copy link

Preview build will be at
https://review-createai.microbit.org/recording-options/

}, dataWindow.duration);
if (isRecording) {
// Set the start recording line
const now = new Date().getTime();
Copy link

@microbit-matt-hillsdon microbit-matt-hillsdon Nov 1, 2024

Choose a reason for hiding this comment

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

I'd feel better about this if this could get the timestamps from the code that controls the recording.
Maybe we have a way to listen for recording events that include timestamps rather than using React state.

const recorder = useRecorder()
useEffect(() => {
  // With clean-up omitted, events carry a timestamp
  recorder.addEventListener("recordingstart", ...)
  recorder.addEventListener("recordingstop", ...)
}, [recorder]);

We can look at this in a follow up PR.

Comment on lines +118 to +126
useEffect(() => {
if (
!recordingDialogDisclosure.isOpen &&
gestures.length === 1 &&
gestures[0].recordings.length === 1
) {
tourStart(TourId.CollectDataToTrainModel);
}
}, [gestures, recordingDialogDisclosure.isOpen, tourStart]);
Copy link
Author

Choose a reason for hiding this comment

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

Would have prefered to do this on recording dialog close, but the latest version of the gesture recordings aren't available at that point.

@microbit-robert microbit-robert changed the title [DRAFT - WIP] Add additional recording options Add additional recording options Nov 1, 2024
@microbit-robert microbit-robert marked this pull request as ready for review November 1, 2024 12:59
Copy link

@microbit-grace microbit-grace left a comment

Choose a reason for hiding this comment

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

LGTM, not sure if @microbit-matt-hillsdon has anymore comments

@microbit-robert microbit-robert merged commit db73768 into main Nov 1, 2024
1 check passed
@microbit-robert microbit-robert deleted the recording-options branch November 1, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants