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

Display informational warning when code is edited while its running #35351

Merged
merged 21 commits into from Jun 29, 2020

Conversation

JillianK
Copy link
Contributor

@JillianK JillianK commented Jun 16, 2020

This displays a warning at the bottom of the workspace if someone makes changes in the workspace while the code is running. This warning will only show up once during a run and go away if they close it or if they click reset (for example, if you run, edit, close the warning, and then edit again you won't get the warning again during this run but it will show up again if you reset, run, and then edit).

Game Lab:
Screen Shot 2020-06-16 at 11 48 59 AM

App Lab on mobile:
IMG_0910

Dance Party:
Screen Shot 2020-06-16 at 11 49 31 AM

Sprite Lab on mobile:
IMG_0909

Craft:
Screen Shot 2020-06-16 at 11 50 00 AM

Craft on mobile:
IMG_0911

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • 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

@JillianK JillianK requested a review from a team as a code owner June 16, 2020 21:02
apps/src/StudioApp.js Outdated Show resolved Hide resolved
apps/src/StudioApp.js Outdated Show resolved Hide resolved
apps/src/StudioApp.js Outdated Show resolved Hide resolved
this.editDuringRunAlert = undefined;
}
} else {
this.executingCode = this.getCode().trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

Still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change handlers are also run when you toggle between code and block mode in droplet so we need the code when the user clicked run to compare against later to see if there was an actual change.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

i can't comment on the file itself, but the change(s) to dashboard/db/schema_cache.dump here should be removed

apps/src/StudioApp.js Outdated Show resolved Hide resolved
Created a WorkspaceAlert wrapper for the alert component.
apps/src/StudioApp.js Outdated Show resolved Hide resolved
var container = $('<div/>');
parent.append(container);
ReactDOM.render(
<WorkspaceAlert
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

please do continue to avoid using JSX syntax inside non-JSX files

Copy link
Contributor

Choose a reason for hiding this comment

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

@Hamms - what's the preferred way to write this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just switched it to use createElement like you suggested earlier, let me know if this wasn't what you meant

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, sorry I missed this. Yep, createElement is preferred! Thanks

Copy link
Contributor

@jmkulwik jmkulwik left a comment

Choose a reason for hiding this comment

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

Great work!!!

@JillianK JillianK merged commit 9bbfb87 into staging Jun 29, 2020
@JillianK JillianK deleted the informational-warning branch June 29, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants