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
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/i18n/common/en_us.json
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,7 @@
"edit": "Edit",
"editAll": "Edit all",
"editProject": "Edit Project",
"editDuringRunMessage": "Your code may have changed. Click \"Reset\" and then \"Run\" to run your code again.",
"editSectionDetails": "Edit Section Details",
"editSectionLoginTypeCleverDesc": "Students sign in through Clever.",
"editSectionLoginTypeEmailDesc": "Students sign in with their personal email login information.",
Expand Down
77 changes: 60 additions & 17 deletions apps/src/StudioApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ import {RESIZE_VISUALIZATION_EVENT} from './lib/ui/VisualizationResizeBar';
import {userAlreadyReportedAbuse} from '@cdo/apps/reportAbuse';
import {setArrowButtonDisabled} from '@cdo/apps/templates/arrowDisplayRedux';
import {workspace_running_background, white} from '@cdo/apps/util/color';
import WorkspaceAlert from '@cdo/apps/code-studio/components/WorkspaceAlert';

var copyrightStrings;

Expand Down Expand Up @@ -196,6 +197,16 @@ class StudioApp extends EventEmitter {
* Levelbuilder-defined helper libraries.
*/
this.libraries = {};

/*
* Stores the alert that appears if the user edits code while its running. It will be unmounted and set to undefined on reset.
*/
this.editDuringRunAlert = undefined;

/*
* Stores the code at run. It's undefined if the code is not running.
*/
this.executingCode = undefined;
JillianK marked this conversation as resolved.
Show resolved Hide resolved
}
}
/**
Expand Down Expand Up @@ -539,6 +550,24 @@ StudioApp.prototype.init = function(config) {
startDialogDiv
);
}
if (!config.readOnlyWorkspace) {
this.addChangeHandler(() => {
// if the code has changed (other than whitespace at the beginning or end) and the code is running,
// we want to show display an alert to tell the user to reset and run their code again. We trim the whitespace
JillianK marked this conversation as resolved.
Show resolved Hide resolved
// because droplet sometimes adds an extra newline when switching from block to code mode.
if (
this.isRunning() &&
this.editDuringRunAlert === undefined &&
this.getCode().trim() !== this.executingCode.trim()
) {
this.editDuringRunAlert = this.displayWorkspaceAlert(
'warning',
React.createElement('div', {}, msg.editDuringRunMessage()),
true
);
}
});
}

this.emit('afterInit');
};
Expand Down Expand Up @@ -944,6 +973,15 @@ StudioApp.prototype.toggleRunReset = function(button) {

getStore().dispatch(setIsRunning(!showRun));

if (showRun) {
if (this.editDuringRunAlert !== undefined) {
ReactDOM.unmountComponentAtNode(this.editDuringRunAlert);
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.

👍

}

if (this.hasContainedLevels) {
lockContainedLevelAnswers();
}
Expand Down Expand Up @@ -3039,25 +3077,30 @@ function rectFromElementBoundingBox(element) {
* @param {string} type - Alert type (error, warning, or notification)
* @param {React.Component} alertContents
*/
StudioApp.prototype.displayWorkspaceAlert = function(type, alertContents) {
var container = this.displayAlert(
'#codeWorkspace',
{type: type},
alertContents
StudioApp.prototype.displayWorkspaceAlert = function(
JillianK marked this conversation as resolved.
Show resolved Hide resolved
type,
alertContents,
bottom = false
) {
var parent = $(bottom && this.editCode ? '#codeTextbox' : '#codeWorkspace');
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

type={type}
onClose={() => {
ReactDOM.unmountComponentAtNode(container[0]);
}}
isBlockly={this.usingBlockly_}
isCraft={this.config.app === 'craft'}
displayBottom={bottom}
>
{alertContents}
</WorkspaceAlert>,
container[0]
);

var toolbarWidth;
if (this.usingBlockly_) {
toolbarWidth = $('.blocklyToolboxDiv').width();
} else {
toolbarWidth =
$('.droplet-palette-element').width() + $('.droplet-gutter').width();
}

$(container).css({
left: toolbarWidth,
top: $('#headers').height()
});
return container[0];
};

/**
Expand Down
54 changes: 54 additions & 0 deletions apps/src/code-studio/components/WorkspaceAlert.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import PropTypes from 'prop-types';
import React from 'react';
import Alert from '@cdo/apps/templates/alert';
import $ from 'jquery';

export default class WorkspaceAlert extends React.Component {
static propTypes = {
type: PropTypes.oneOf(['error', 'warning', 'notification']).isRequired,
JillianK marked this conversation as resolved.
Show resolved Hide resolved
children: PropTypes.element,
onClose: PropTypes.func,
JillianK marked this conversation as resolved.
Show resolved Hide resolved
isBlockly: PropTypes.bool,
isCraft: PropTypes.bool,
displayBottom: PropTypes.bool
};

render() {
const {displayBottom} = this.props;
JillianK marked this conversation as resolved.
Show resolved Hide resolved
var toolbarWidth;
if (this.props.isBlockly && this.props.isCraft) {
// craft has a slightly different way of constructing the toolbox so we need to use
// the toolbox header's width to get the width of the actual toolbox.
toolbarWidth = $('#toolbox-header').width();
} else if (this.props.isBlockly) {
toolbarWidth = $('.blocklyToolboxDiv').width();
} else {
toolbarWidth =
$('.droplet-palette-element').width() + $('.droplet-gutter').width();
}

let style = {
position: 'absolute',
right: 0,
left: toolbarWidth
};
JillianK marked this conversation as resolved.
Show resolved Hide resolved
if (displayBottom) {
style.bottom = 0;
} else {
style.top = $('#headers').height();
}

return (
<div style={style}>
<Alert
onClose={this.props.onClose}
type={this.props.type}
sideMargin={displayBottom ? 0 : undefined}
bottomMargin={displayBottom ? 0 : undefined}
>
{this.props.children}
</Alert>
</div>
);
}
}
3 changes: 2 additions & 1 deletion apps/src/templates/alert.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default class Alert extends React.Component {
onClose: PropTypes.func.isRequired,
closeDelayMillis: PropTypes.number,
sideMargin: PropTypes.number,
bottomMargin: PropTypes.number,
childPadding: PropTypes.string
};

Expand Down Expand Up @@ -45,7 +46,7 @@ export default class Alert extends React.Component {
child: {
// from bootstrap's alert
padding: valueOr(this.props.childPadding, '8px 35px 8px 14px'),
marginBottom: 20,
marginBottom: valueOr(this.props.bottomMargin, 20),
textShadoow: '0 1px 0 rgba(255, 255, 255, 0.5)',
border: '1px solid',
borderRadius: 4
Expand Down
11 changes: 9 additions & 2 deletions apps/test/unit/StudioAppTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,15 @@ describe('StudioApp', () => {
});

describe('addChangeHandler', () => {
beforeEach(stubStudioApp);
afterEach(restoreStudioApp);
beforeEach(() => {
stubStudioApp();
stubRedux();
registerReducers(commonReducers);
});
afterEach(() => {
restoreRedux();
restoreStudioApp();
});

it('calls a handler in response to a blockly change', () => {
let changed = false;
Expand Down
100 changes: 100 additions & 0 deletions apps/test/unit/code-studio/components/WorkspaceAlertTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import React from 'react';
import {expect} from '../../../util/deprecatedChai';
JillianK marked this conversation as resolved.
Show resolved Hide resolved
import {mount} from 'enzyme';
import WorkspaceAlert from '@cdo/apps/code-studio/components/WorkspaceAlert';
import $ from 'jquery';
import sinon from 'sinon';

describe('WorkspaceAlert', () => {
it('can be at the top or bottom', () => {
JillianK marked this conversation as resolved.
Show resolved Hide resolved
var jQueryHeight = sinon.stub($.fn, 'height').returns(4);
JillianK marked this conversation as resolved.
Show resolved Hide resolved
const top = mount(
<WorkspaceAlert
type="warning"
onClose={() => {}}
isBlockly={true}
isCraft={false}
displayBottom={false}
>
<span>This is a top alert</span>
</WorkspaceAlert>
);
expect(jQueryHeight.callCount).to.equal(1);
expect(jQueryHeight.thisValues[0].selector).to.equal('#headers');
expect(top).to.have.style('top', '4px');

const bottom = mount(
<WorkspaceAlert
type="warning"
onClose={() => {}}
isBlockly={true}
isCraft={false}
displayBottom={true}
>
<span>This is a bottom alert</span>
</WorkspaceAlert>
);
expect(bottom).to.have.style('bottom', '0px');
});

it('left changes based on isBlockly and isCraft', () => {
JillianK marked this conversation as resolved.
Show resolved Hide resolved
var jQueryWidth = sinon
.stub($.fn, 'width')
.onCall(0)
.returns(1)
.onCall(1)
.returns(2)
.onCall(2)
.returns(3)
.onCall(3)
.returns(4)
.returns(0);
const isBlocklyAndisCraft = mount(
<WorkspaceAlert
type="warning"
onClose={() => {}}
isBlockly={true}
isCraft={true}
displayBottom={true}
>
<span>This is a craft and blockly alert</span>
</WorkspaceAlert>
);
expect(jQueryWidth.callCount).to.equal(1);
expect(jQueryWidth.thisValues[0].selector).to.equal('#toolbox-header');
expect(isBlocklyAndisCraft).to.have.style('left', '1px');

const isBlockly = mount(
<WorkspaceAlert
type="warning"
onClose={() => {}}
isBlockly={true}
isCraft={false}
displayBottom={true}
>
<span>This is a blockly alert</span>
</WorkspaceAlert>
);
expect(jQueryWidth.callCount).to.equal(2);
expect(jQueryWidth.thisValues[1].selector).to.equal('.blocklyToolboxDiv');
expect(isBlockly).to.have.style('left', '2px');

const neither = mount(
<WorkspaceAlert
type="warning"
onClose={() => {}}
isBlockly={false}
isCraft={false}
displayBottom={true}
>
<span>This is a neither craft nor blockly alert</span>
</WorkspaceAlert>
);
expect(neither).to.have.style('left', '7px');
expect(jQueryWidth.callCount).to.equal(4);
expect(jQueryWidth.thisValues[2].selector).to.equal(
'.droplet-palette-element'
);
expect(jQueryWidth.thisValues[3].selector).to.equal('.droplet-gutter');
});
});