Skip to content

Commit

Permalink
Rework Composer exit prevention logic
Browse files Browse the repository at this point in the history
- The main breakthrough was moving the logic from Composer to
  ComposerBody. The former had to consult the state for logic
  and values readily available inside the ComposerBody (and
  its subclasses).
- The state is no longer involved in the browser-level unload
  event handling.
- It still has to take care of the manual confirmation message
  that is shown when the composer itself is closed while
  drafting a message.
- The former does not need a custom message (it would ignore
  it anyway), the latter does.
- The callback and the text that should be displayed are now
  passed to the state with a nicely encapsulating method.
- While I was at it, I extracted a simple helper component
  for handling the event (de)registration. JSX composition is
  so nice!
  • Loading branch information
franzliedke committed Jul 17, 2020
1 parent 9429fb2 commit 050081c
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 45 deletions.
37 changes: 37 additions & 0 deletions js/src/common/components/ConfirmDocumentUnload.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import Component from '../Component';

/**
* The `ConfirmDocumentUnload` component can be used to register a global
* event handler that prevents closing the browser window/tab based on the
* return value of a given callback prop.
*
* ### Props
*
* - `when` - a callback returning true when the browser should prompt for
* confirmation before closing the window/tab
*
* ### Children
*
* NOTE: Only the first child will be rendered. (Use this component to wrap
* another component / DOM element.)
*
*/
export default class ConfirmDocumentUnload extends Component {
config(isInitialized, context) {
if (isInitialized) return;

const handler = () => this.props.when() || undefined;

$(window).on('beforeunload', handler);

context.onunload = () => {
$(window).off('beforeunload', handler);
};
}

view() {
// To avoid having to render another wrapping <div> here, we assume that
// this component is only wrapped around a single element / component.
return this.props.children[0];
}
}
12 changes: 1 addition & 11 deletions js/src/forum/components/Composer.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,6 @@ export default class Composer extends Component {

const handlers = {};

// Don't let the user leave the page without first giving the composer's
// component a chance to scream at the user to make sure they don't
// unintentionally lose any content.
$(window).on(
'beforeunload',
(handlers.onbeforeunload = () => {
return (this.state.bodyPreventExit && this.state.bodyPreventExit()) || undefined;
})
);

$(window)
.on('resize', (handlers.onresize = this.updateHeight.bind(this)))
.resize();
Expand All @@ -105,7 +95,7 @@ export default class Composer extends Component {
.on('mouseup', (handlers.onmouseup = this.onmouseup.bind(this)));

context.onunload = () => {
$(window).off('resize', handlers.onresize).off('beforeunload', handlers.onbeforeunload);
$(window).off('resize', handlers.onresize);

$(document).off('mousemove', handlers.onmousemove).off('mouseup', handlers.onmouseup);
};
Expand Down
41 changes: 22 additions & 19 deletions js/src/forum/components/ComposerBody.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Component from '../../common/Component';
import LoadingIndicator from '../../common/components/LoadingIndicator';
import ConfirmDocumentUnload from '../../common/components/ConfirmDocumentUnload';
import TextEditor from './TextEditor';
import avatar from '../../common/helpers/avatar';
import listItems from '../../common/helpers/listItems';
Expand Down Expand Up @@ -33,7 +34,7 @@ export default class ComposerBody extends Component {
*/
this.loading = false;

this.composer.bodyPreventExit = this.preventExit.bind(this);
this.composer.preventClosingWhen(() => this.preventExit(), this.props.confirmExit);

this.composer.content(this.props.originalContent || '');

Expand All @@ -46,25 +47,27 @@ export default class ComposerBody extends Component {

view() {
return (
<div className={'ComposerBody ' + (this.props.className || '')}>
{avatar(this.props.user, { className: 'ComposerBody-avatar' })}
<div className="ComposerBody-content">
<ul className="ComposerBody-header">{listItems(this.headerItems().toArray())}</ul>
<div className="ComposerBody-editor">
{TextEditor.component({
submitLabel: this.props.submitLabel,
placeholder: this.props.placeholder,
disabled: this.loading || this.props.disabled,
composer: this.composer,
preview: this.preview.bind(this),
onchange: this.composer.content,
onsubmit: this.onsubmit.bind(this),
value: this.composer.content(),
})}
<ConfirmDocumentUnload when={this.preventExit.bind(this)}>
<div className={'ComposerBody ' + (this.props.className || '')}>
{avatar(this.props.user, { className: 'ComposerBody-avatar' })}
<div className="ComposerBody-content">
<ul className="ComposerBody-header">{listItems(this.headerItems().toArray())}</ul>
<div className="ComposerBody-editor">
{TextEditor.component({
submitLabel: this.props.submitLabel,
placeholder: this.props.placeholder,
disabled: this.loading || this.props.disabled,
composer: this.composer,
preview: this.preview.bind(this),
onchange: this.composer.content,
onsubmit: this.onsubmit.bind(this),
value: this.composer.content(),
})}
</div>
</div>
{LoadingIndicator.component({ className: 'ComposerBody-loading' + (this.loading ? ' active' : '') })}
</div>
{LoadingIndicator.component({ className: 'ComposerBody-loading' + (this.loading ? ' active' : '') })}
</div>
</ConfirmDocumentUnload>
);
}

Expand All @@ -77,7 +80,7 @@ export default class ComposerBody extends Component {
preventExit() {
const content = this.composer.content();

return content && content !== this.props.originalContent && this.props.confirmExit;
return content && content !== this.props.originalContent;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion js/src/forum/components/DiscussionComposer.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export default class DiscussionComposer extends ComposerBody {
}

preventExit() {
return (this.title() || this.composer.content()) && this.props.confirmExit;
return this.title() || this.composer.content();
}

/**
Expand Down
32 changes: 18 additions & 14 deletions js/src/forum/states/ComposerState.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,6 @@ class ComposerState {
*/
this.$texteditor = null;

/**
* A callback to be executed before the composer is closed.
*
* This can be used to prevent e.g. losing unsaved content.
*
* @type {Function|null}
*/
this.bodyPreventExit = null;

this.clear();

/**
Expand Down Expand Up @@ -81,7 +72,7 @@ class ComposerState {
this.position = ComposerState.Position.HIDDEN;
this.body = { attrs: {} };
this.$texteditor = null;
this.bodyPreventExit = null;
this.onExit = null;

this.fields = {
content: m.prop(''),
Expand Down Expand Up @@ -237,14 +228,27 @@ class ComposerState {
*/
preventExit() {
if (!this.isVisible()) return;
if (!this.onExit) return;

const preventExit = this.bodyPreventExit();

if (preventExit) {
return !confirm(preventExit);
if (this.onExit.callback()) {
return !confirm(this.onExit.message);
}
}

/**
* Configure when / what to ask the user before closing the composer.
*
* The provided callback will be used to determine whether asking for
* confirmation is necessary. If the callback returns true at the time of
* closing, the provided text will be shown in a standard confirmation dialog.
*
* @param {Function} callback
* @param {String} message
*/
preventClosingWhen(callback, message) {
this.onExit = { callback, message };
}

/**
* Minimum height of the Composer.
* @returns {Integer}
Expand Down

0 comments on commit 050081c

Please sign in to comment.