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

[Spritelab] Implement Pause button behind experiment #38457

Merged
merged 6 commits into from Jan 8, 2021

Conversation

ajpal
Copy link
Contributor

@ajpal ajpal commented Jan 7, 2021

Work in progress/prototype for the ability to pause spritelab execution
This mostly hooks into the existing break/pause functionality in gamelab.
Behind an experiment (spritelabPause) for now mostly so that curriculum writers can start playing with it / writing lessons as we continue to iterate together.
image

Jan-07-2021 15-45-16

Not done yet:

  • a UI that went through any sort of design/spec process
  • figure out whether we can track pause time in p5.play instead of the spritelab library. That way, it would work for gamelab as well.
  • Unit tests. I expect the functionality and definitely the UI to change somewhat before we remove the experiment, so holding off on tests until we have a more solid agreement on expected behavior.

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@ajpal ajpal requested a review from a team January 7, 2021 23:49
@@ -192,6 +193,9 @@ class P5LabVisualizationColumn extends React.Component {
</div>

<GameButtons>
{experiments.isEnabled(experiments.SPRITELAB_PAUSE) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

this checks localStorage every time the component renders, which has caused performance issues in the past, so it should be moved to a piece of state set during componentDidMount or stored in some other way (memoized or something similar would also work)

togglePause = () => {
const current = new Date().getTime();
if (this.props.isPaused) {
coreLibrary.addPauseTime(current - this.state.pauseStart);
Copy link
Contributor

Choose a reason for hiding this comment

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

this function should probably be a prop so that PauseButton doesn't depend on coreLibrary and can be reused

@ajpal ajpal merged commit 310a226 into staging Jan 8, 2021
@ajpal ajpal deleted the jan7-spritelab-pause branch January 8, 2021 22:39
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.

None yet

2 participants