Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
apply review feedback from @lukebarnard1
Browse files Browse the repository at this point in the history
  • Loading branch information
@ara4n
ara4n committed Jul 8, 2018
1 parent e7e4ee8 commit 37d4bce
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 116 deletions.
36 changes: 0 additions & 36 deletions src/HtmlUtils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,42 +112,6 @@ export function charactersToImageNode(alt, useSvg, ...unicode) {
/>;
}

/*
export function processHtmlForSending(html: string): string {
const contentDiv = document.createElement('div');
contentDiv.innerHTML = html;
if (contentDiv.children.length === 0) {
return contentDiv.innerHTML;
}
let contentHTML = "";
for (let i=0; i < contentDiv.children.length; i++) {
const element = contentDiv.children[i];
if (element.tagName.toLowerCase() === 'p') {
contentHTML += element.innerHTML;
// Don't add a <br /> for the last <p>
if (i !== contentDiv.children.length - 1) {
contentHTML += '<br />';
}
} else if (element.tagName.toLowerCase() === 'pre') {
// Replace "<br>\n" with "\n" within `<pre>` tags because the <br> is
// redundant. This is a workaround for a bug in draft-js-export-html:
// https://github.com/sstur/draft-js-export-html/issues/62
contentHTML += '<pre>' +
element.innerHTML.replace(/<br>\n/g, '\n').trim() +
'</pre>';
} else {
const temp = document.createElement('div');
temp.appendChild(element.cloneNode(true));
contentHTML += temp.innerHTML;
}
}
return contentHTML;
}
*/

/*
* Given an untrusted HTML string, return a React node with an sanitized version
* of that HTML.
Expand Down
8 changes: 0 additions & 8 deletions src/Markdown.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,14 +180,6 @@ export default class Markdown {
if (is_multi_line(node) && node.next) this.lit('\n\n');
};

// convert MD links into console-friendly ' < http://foo >' style links
// ...except given this function never gets called with links, it's useless.
// renderer.link = function(node, entering) {
// if (!entering) {
// this.lit(` < ${node.destination} >`);
// }
// };

return renderer.render(this.parsed);
}
}
6 changes: 3 additions & 3 deletions src/autocomplete/Autocompleter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ import NotifProvider from './NotifProvider';
import Promise from 'bluebird';

export type SelectionRange = {
beginning: boolean,
start: number,
end: number
beginning: boolean, // whether the selection is in the first block of the editor or not
start: number, // byte offset relative to the start anchor of the current editor selection.
end: number, // byte offset relative to the end anchor of the current editor selection.
};

export type Completion = {
Expand Down
2 changes: 1 addition & 1 deletion src/autocomplete/UserProvider.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export default class UserProvider extends AutocompleteProvider {
// relies on the length of the entity === length of the text in the decoration.
completion: user.rawDisplayName.replace(' (IRC)', ''),
completionId: user.userId,
suffix: (selection.beginning && range.start === 0) ? ': ' : ' ',
suffix: (selection.beginning && selection.start === 0) ? ': ' : ' ',
href: makeUserPermalink(user.userId),
component: (
<PillCompletion
Expand Down
94 changes: 30 additions & 64 deletions src/components/views/rooms/MessageComposerInput.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,14 @@ import type SyntheticKeyboardEvent from 'react/lib/SyntheticKeyboardEvent';

import { Editor } from 'slate-react';
import { getEventTransfer } from 'slate-react';
import { Value, Document, Event, Block, Inline, Text, Range, Node } from 'slate';
import { Value, Document, Block, Inline, Text, Range, Node } from 'slate';
import type { Change } from 'slate';

import Html from 'slate-html-serializer';
import Md from 'slate-md-serializer';
import Plain from 'slate-plain-serializer';
import PlainWithPillsSerializer from "../../../autocomplete/PlainWithPillsSerializer";

// import {Editor, EditorState, RichUtils, CompositeDecorator, Modifier,
// getDefaultKeyBinding, KeyBindingUtil, ContentState, ContentBlock, SelectionState,
// Entity} from 'draft-js';

import classNames from 'classnames';
import Promise from 'bluebird';

Expand Down Expand Up @@ -170,13 +166,25 @@ export default class MessageComposerInput extends React.Component {

this.client = MatrixClientPeg.get();

// track whether we should be trying to show autocomplete suggestions on the current editor
// contents. currently it's only suppressed when navigating history to avoid ugly flashes
// of unexpected corrections as you navigate.
// XXX: should this be in state?
this.suppressAutoComplete = false;

// track whether we've just pressed an arrowkey left or right in order to skip void nodes.
// see https://github.com/ianstormtaylor/slate/issues/762#issuecomment-304855095
this.direction = '';

this.plainWithMdPills = new PlainWithPillsSerializer({ pillFormat: 'md' });
this.plainWithIdPills = new PlainWithPillsSerializer({ pillFormat: 'id' });
this.plainWithPlainPills = new PlainWithPillsSerializer({ pillFormat: 'plain' });

this.md = new Md({
rules: [
{
// if serialize returns undefined it falls through to the default hardcoded
// serialization rules
serialize: (obj, children) => {
if (obj.object !== 'inline') return;
switch (obj.type) {
Expand Down Expand Up @@ -288,17 +296,15 @@ export default class MessageComposerInput extends React.Component {
}
]
});

this.suppressAutoComplete = false;
this.direction = '';
}

/*
* "Does the right thing" to create an Editor value, based on:
* - whether we've got rich text mode enabled
* - contentState was passed in
*/
createEditorState(richText: boolean, editorState: ?Value): Value {
createEditorState(richText: boolean, // eslint-disable-line no-unused-vars
editorState: ?Value): Value {
if (editorState instanceof Value) {
return editorState;
}
Expand Down Expand Up @@ -370,7 +376,6 @@ export default class MessageComposerInput extends React.Component {
let fragmentChange = fragment.change();
fragmentChange.moveToRangeOf(fragment.document)
.wrapBlock(quote);
//.setBlocks('block-quote');

// FIXME: handle pills and use commonmark rather than md-serialize
const md = this.md.serialize(fragmentChange.value);
Expand Down Expand Up @@ -458,6 +463,7 @@ export default class MessageComposerInput extends React.Component {
if (this.direction !== '') {
const focusedNode = editorState.focusInline || editorState.focusText;
if (focusedNode.isVoid) {
// XXX: does this work in RTL?
const edge = this.direction === 'Previous' ? 'End' : 'Start';
if (editorState.isCollapsed) {
change = change[`collapseTo${ edge }Of${ this.direction }Text`]();
Expand All @@ -477,13 +483,6 @@ export default class MessageComposerInput extends React.Component {
this.onFinishedTyping();
}

/*
// XXX: what was this ever doing?
if (!state.hasOwnProperty('originalEditorState')) {
state.originalEditorState = null;
}
*/

if (editorState.startText !== null) {
const text = editorState.startText.text;
const currentStartOffset = editorState.startOffset;
Expand Down Expand Up @@ -511,9 +510,7 @@ export default class MessageComposerInput extends React.Component {
}

// emojioneify any emoji

// XXX: is getTextsAsArray a private API?
editorState.document.getTextsAsArray().forEach(node => {
editorState.document.getTexts().forEach(node => {
if (node.text !== '' && HtmlUtils.containsEmoji(node.text)) {
let match;
while ((match = EMOJI_REGEX.exec(node.text)) !== null) {
Expand Down Expand Up @@ -545,36 +542,6 @@ export default class MessageComposerInput extends React.Component {
editorState = change.value;
}

/*
const currentBlock = editorState.getSelection().getStartKey();
const currentSelection = editorState.getSelection();
const currentStartOffset = editorState.getSelection().getStartOffset();
const block = editorState.getCurrentContent().getBlockForKey(currentBlock);
const text = block.getText();
const entityBeforeCurrentOffset = block.getEntityAt(currentStartOffset - 1);
const entityAtCurrentOffset = block.getEntityAt(currentStartOffset);
// If the cursor is on the boundary between an entity and a non-entity and the
// text before the cursor has whitespace at the end, set the entity state of the
// character before the cursor (the whitespace) to null. This allows the user to
// stop editing the link.
if (entityBeforeCurrentOffset && !entityAtCurrentOffset &&
/\s$/.test(text.slice(0, currentStartOffset))) {
editorState = RichUtils.toggleLink(
editorState,
currentSelection.merge({
anchorOffset: currentStartOffset - 1,
focusOffset: currentStartOffset,
}),
null,
);
// Reset selection
editorState = EditorState.forceSelection(editorState, currentSelection);
}
*/

if (this.props.onInputStateChanged && editorState.blocks.size > 0) {
let blockType = editorState.blocks.first().type;
// console.log("onInputStateChanged; current block type is " + blockType + " and marks are " + editorState.activeMarks);
Expand Down Expand Up @@ -604,7 +571,6 @@ export default class MessageComposerInput extends React.Component {
editor_state: editorState,
});

/* Since a modification was made, set originalEditorState to null, since newState is now our original */
this.setState({
editorState,
originalEditorState: originalEditorState || null
Expand Down Expand Up @@ -682,7 +648,7 @@ export default class MessageComposerInput extends React.Component {

hasMark = type => {
const { editorState } = this.state
return editorState.activeMarks.some(mark => mark.type == type)
return editorState.activeMarks.some(mark => mark.type === type)
};

/**
Expand All @@ -694,7 +660,7 @@ export default class MessageComposerInput extends React.Component {

hasBlock = type => {
const { editorState } = this.state
return editorState.blocks.some(node => node.type == type)
return editorState.blocks.some(node => node.type === type)
};

onKeyDown = (ev: KeyboardEvent, change: Change, editor: Editor) => {
Expand Down Expand Up @@ -827,7 +793,7 @@ export default class MessageComposerInput extends React.Component {
// Handle the extra wrapping required for list buttons.
const isList = this.hasBlock('list-item');
const isType = editorState.blocks.some(block => {
return !!document.getClosest(block.key, parent => parent.type == type);
return !!document.getClosest(block.key, parent => parent.type === type);
});

if (isList && isType) {
Expand All @@ -838,7 +804,7 @@ export default class MessageComposerInput extends React.Component {
} else if (isList) {
change
.unwrapBlock(
type == 'bulleted-list' ? 'numbered-list' : 'bulleted-list'
type === 'bulleted-list' ? 'numbered-list' : 'bulleted-list'
)
.wrapBlock(type);
} else {
Expand Down Expand Up @@ -1008,7 +974,7 @@ export default class MessageComposerInput extends React.Component {
let contentHTML;

// only look for commands if the first block contains simple unformatted text
// i.e. no pills or rich-text formatting.
// i.e. no pills or rich-text formatting and begins with a /.
let cmd, commandText;
const firstChild = editorState.document.nodes.get(0);
const firstGrandChild = firstChild && firstChild.nodes.get(0);
Expand Down Expand Up @@ -1089,8 +1055,8 @@ export default class MessageComposerInput extends React.Component {
// didn't contain any formatting in the first place...
contentText = mdWithPills.toPlaintext();
} else {
// to avoid ugliness clients which can't parse HTML we don't send pills
// in the plaintext body.
// to avoid ugliness on clients which ignore the HTML body we don't
// send pills in the plaintext body.
contentText = this.plainWithPlainPills.serialize(editorState);
contentHTML = mdWithPills.toHTML();
}
Expand Down Expand Up @@ -1254,11 +1220,8 @@ export default class MessageComposerInput extends React.Component {
// Move selection to the end of the selected history
const change = editorState.change().collapseToEndOf(editorState.document);

// XXX: should we be calling this.onChange(change) now?
// Answer: yes, if we want it to do any of the fixups on stuff like emoji.
// however, this should already have been done and persisted in the history,
// so shouldn't be necessary.

// We don't call this.onChange(change) now, as fixups on stuff like emoji
// should already have been done and persisted in the history.
editorState = change.value;

this.suppressAutoComplete = true;
Expand Down Expand Up @@ -1361,6 +1324,8 @@ export default class MessageComposerInput extends React.Component {
.insertText(suffix)
.focus();
}
// for good hygiene, keep editorState updated to track the result of the change
// even though we don't do anything subsequently with it
editorState = change.value;

this.onChange(change, activeEditorState);
Expand Down Expand Up @@ -1459,10 +1424,11 @@ export default class MessageComposerInput extends React.Component {
};

onFormatButtonClicked = (name, e) => {
e.preventDefault(); // don't steal focus from the editor!
e.preventDefault();

// XXX: horrible evil hack to ensure the editor is focused so the act
// of focusing it doesn't then cancel the format button being pressed
// FIXME: can we just tell handleKeyCommand's change to invoke .focus()?
if (document.activeElement && document.activeElement.className !== 'mx_MessageComposer_editor') {
this.refs.editor.focus();
setTimeout(()=>{
Expand Down
8 changes: 4 additions & 4 deletions test/components/views/rooms/MessageComposerInput-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const MessageComposerInput = sdk.getComponent('views.rooms.MessageComposerInput'
import MatrixClientPeg from '../../../../src/MatrixClientPeg';
import RoomMember from 'matrix-js-sdk';

/*
function addTextToDraft(text) {
const components = document.getElementsByClassName('public-DraftEditor-content');
if (components && components.length) {
Expand All @@ -21,7 +20,9 @@ function addTextToDraft(text) {
}
}

describe('MessageComposerInput', () => {
// FIXME: These tests need to be updated from Draft to Slate.

xdescribe('MessageComposerInput', () => {
let parentDiv = null,
sandbox = null,
client = null,
Expand Down Expand Up @@ -300,5 +301,4 @@ describe('MessageComposerInput', () => {
expect(spy.args[0][1].body).toEqual('[Click here](https://some.lovely.url)');
expect(spy.args[0][1].formatted_body).toEqual('<a href="https://some.lovely.url">Click here</a>');
});
});
*/
});

0 comments on commit 37d4bce

Please sign in to comment.