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

Allow for undo burst functionality (#36) #37

Merged
merged 6 commits into from
Jul 8, 2019
Merged

Allow for undo burst functionality (#36) #37

merged 6 commits into from
Jul 8, 2019

Conversation

colebillys19
Copy link
Contributor

Implemented 'undo burst' functionality (issue #36).

Summary:

  • added functions to get and set calculator state to the calc-helpers file
  • mapped frame and frameIDs (from the images state slice) to the Burst component's props
  • changed the Burst component's local state to include prevFrames, prevFrameIDs, and prevCalcState to allow for undo
  • in handleRequestBurst, im grabbing these three values prior to burst capture
  • if the capture is successful, the data is saved in local state
  • if unsuccessful, data is not saved
  • also added isCapturing to local state to trigger UI change when burst is being captured... capture button changes to 'capturing...' and is unclickable
  • when a burst has been captured, an undo button appears in the UI
  • added undoBurst action creator, UNDO_BURST action type
  • added handleUndoBurst method, dispatches action and reverts calculator

@colebillys19
Copy link
Contributor Author

Jason pointed that the undo burst button was not showing when you capture a burst with no frames previously captured. This was because the render method for Burst is conditionally rendering the button based on whether or not there were frames in state before the capture. Not the best logic. I added a straightforward boolean called canUndo that is used to check whether or not the button should be rendered. This is set to true on burst capture, and set to false on undo.

async handleRequestBurst() {
this.setState({ isCapturing: true });
const { requestBurst, expanded, frames, frameIDs, ...imgOpts } = this.props;
// grab calculator state prior to capture
Copy link
Contributor

@japamat0 japamat0 Jun 19, 2019

Choose a reason for hiding this comment

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

With some of your nicely named function names, some of these comments may not be necessary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -39,3 +39,13 @@ export const setSliderByIndex = (idx, val) => {
const identifier = match[1];
calculator.setExpression({ id, latex: `${identifier}=${val}` });
};

// gets current state of calculator instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments may not be necessary for these functions, they are perfectly described by their name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@misscoded misscoded left a comment

Choose a reason for hiding this comment

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

It's possible to interrupt the capture mid-way through processing with the 'UNDO' button, resulting in only a partial burst. I wouldn't display the option to undo until the burst capture has been completed.

@colebillys19
Copy link
Contributor Author

Fixed

@colebillys19
Copy link
Contributor Author

I also added a componentDidUpdate to the burst component to check when new frames (individual frames) are added to state after a burst is captured. This way once a user captures a new frame with the camera button, indicating they're happy with the burst, the undo burst button goes away.

@ctlusto ctlusto merged commit 82f0e6e into desmosinc:master Jul 8, 2019
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