From b19659f5acbcc85544ecfd940d1813076979b572 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Thu, 22 Aug 2019 14:11:36 -0700 Subject: [PATCH] Address beta feedback * Use signal blue for search box focus outline * Reduce debounce for draft saves * Be less aggressive in our scrolling corrections * Lightbox: Ensure that a tall image is still fully visible * Fix spell checking after Electron API breaking changes * Fix link preview image generation * Message highlight: Move to border in signal blue --- js/spell_check.js | 19 +++---- js/views/conversation_view.js | 15 +++--- patches/react-virtualized+9.21.0.patch | 50 +++++++++++-------- stylesheets/_modules.scss | 69 ++++++++++++++------------ test/spellcheck_test.js | 13 ++++- ts/components/Lightbox.tsx | 2 + ts/components/conversation/Image.tsx | 8 +-- ts/components/conversation/Message.tsx | 17 ++----- ts/util/lint/exceptions.json | 4 +- 9 files changed, 108 insertions(+), 89 deletions(-) diff --git a/js/spell_check.js b/js/spell_check.js index 3e65d7290..cb5d81c71 100644 --- a/js/spell_check.js +++ b/js/spell_check.js @@ -96,11 +96,12 @@ if (process.platform === 'linux') { } const simpleChecker = { - spellCheck(text) { - return !this.isMisspelled(text); + spellCheck(words, callback) { + const mispelled = words.filter(word => this.isMisspelled(word)); + callback(mispelled); }, - isMisspelled(text) { - const misspelled = spellchecker.isMisspelled(text); + isMisspelled(word) { + const misspelled = spellchecker.isMisspelled(word); // The idea is to make this as fast as possible. For the many, many calls which // don't result in the red squiggly, we minimize the number of checks. @@ -109,7 +110,7 @@ const simpleChecker = { } // Only if we think we've found an error do we check the locale and skip list. - if (locale.match(EN_VARIANT) && _.contains(ENGLISH_SKIP_WORDS, text)) { + if (locale.match(EN_VARIANT) && _.contains(ENGLISH_SKIP_WORDS, word)) { return false; } @@ -118,14 +119,14 @@ const simpleChecker = { getSuggestions(text) { return spellchecker.getCorrectionsForMisspelling(text); }, - add(text) { - spellchecker.add(text); + add(word) { + spellchecker.add(word); }, }; const dummyChecker = { - spellCheck() { - return true; + spellCheck(words, callback) { + callback([]); }, isMisspelled() { return false; diff --git a/js/views/conversation_view.js b/js/views/conversation_view.js index cc1c833bf..36c532d86 100644 --- a/js/views/conversation_view.js +++ b/js/views/conversation_view.js @@ -186,7 +186,7 @@ this.maybeGrabLinkPreview.bind(this), 200 ); - this.debouncedSaveDraft = _.debounce(this.saveDraft.bind(this), 2000); + this.debouncedSaveDraft = _.debounce(this.saveDraft.bind(this), 500); this.render(); @@ -2668,18 +2668,18 @@ ); } - const data = await this.makeChunkedRequest(imageUrl); + const chunked = await this.makeChunkedRequest(imageUrl); // Ensure that this file is either small enough or is resized to meet our // requirements for attachments const withBlob = await this.autoScale({ - contentType: data.contentType, - file: new Blob([data.data], { - type: data.contentType, + contentType: chunked.contentType, + file: new Blob([chunked.data], { + type: chunked.contentType, }), }); - const attachment = await this.arrayBufferFromFile(withBlob); + const data = await this.arrayBufferFromFile(withBlob.file); objectUrl = URL.createObjectURL(withBlob.file); const dimensions = await Signal.Types.VisualAttachment.getImageDimensions( @@ -2690,7 +2690,8 @@ ); image = { - ...attachment, + data, + size: data.byteLength, ...dimensions, contentType: withBlob.file.type, }; diff --git a/patches/react-virtualized+9.21.0.patch b/patches/react-virtualized+9.21.0.patch index 974897214..8a064ec0e 100644 --- a/patches/react-virtualized+9.21.0.patch +++ b/patches/react-virtualized+9.21.0.patch @@ -24,7 +24,7 @@ index d9716a0..e7a9f9f 100644 } } diff --git a/node_modules/react-virtualized/dist/commonjs/Grid/Grid.js b/node_modules/react-virtualized/dist/commonjs/Grid/Grid.js -index e1b959a..1e3a269 100644 +index e1b959a..18f8f1d 100644 --- a/node_modules/react-virtualized/dist/commonjs/Grid/Grid.js +++ b/node_modules/react-virtualized/dist/commonjs/Grid/Grid.js @@ -132,6 +132,9 @@ var Grid = function (_React$PureComponent) { @@ -104,11 +104,27 @@ index e1b959a..1e3a269 100644 }); this._maybeCallOnScrollbarPresenceChange(); -@@ -584,6 +611,51 @@ var Grid = function (_React$PureComponent) { +@@ -584,6 +611,59 @@ var Grid = function (_React$PureComponent) { } } -+ if (scrollToColumn >= 0) { ++ // We reset our hasScrolled flags if our target has changed, or if target is not longer set ++ if (scrollToColumn !== prevProps.scrollToColumn || scrollToColumn == null || scrollToColumn < 0) { ++ this._hasScrolledToColumnTarget = false; ++ } ++ if (scrollToRow !== prevProps.scrollToRow || scrollToRow == null || scrollToRow < 0) { ++ this._hasScrolledToRowTarget = false; ++ } ++ ++ // We deactivate our forced scrolling if the user scrolls ++ if (scrollLeft !== prevState.scrollLeft && scrollToColumn >= 0 && scrollPositionChangeReason === SCROLL_POSITION_CHANGE_REASONS.OBSERVED) { ++ this._hasScrolledToColumnTarget = true; ++ } ++ if (scrollTop !== prevState.scrollTop && scrollToRow >= 0 && scrollPositionChangeReason === SCROLL_POSITION_CHANGE_REASONS.OBSERVED) { ++ this._hasScrolledToRowTarget = true; ++ } ++ ++ if (scrollToColumn >= 0 && !this._hasScrolledToColumnTarget) { + const scrollRight = scrollLeft + width; + const targetColumn = instanceProps.columnSizeAndPositionManager.getSizeAndPositionOfCell(scrollToColumn); + @@ -124,13 +140,9 @@ index e1b959a..1e3a269 100644 + const totalColumnsWidth = instanceProps.columnSizeAndPositionManager.getTotalSize(); + const maxScroll = totalColumnsWidth - width; + this._hasScrolledToColumnTarget = (scrollLeft >= maxScroll || targetColumn.offset === scrollLeft); -+ } else if (scrollToColumn !== prevProps.scrollToColumn) { -+ this._hasScrolledToColumnTarget = false; + } -+ } else { -+ this._hasScrolledToColumnTarget = false; + } -+ if (scrollToRow >= 0) { ++ if (scrollToRow >= 0 && !this._hasScrolledToRowTarget) { + const scrollBottom = scrollTop + height; + const targetRow = instanceProps.rowSizeAndPositionManager.getSizeAndPositionOfCell(scrollToRow); + @@ -146,17 +158,13 @@ index e1b959a..1e3a269 100644 + const totalRowsHeight = instanceProps.rowSizeAndPositionManager.getTotalSize(); + const maxScroll = totalRowsHeight - height; + this._hasScrolledToRowTarget = (scrollTop >= maxScroll || targetRow.offset === scrollTop); -+ } else if (scrollToRow !== prevProps.scrollToRow) { -+ this._hasScrolledToRowTarget = false; + } -+ } else { -+ this._hasScrolledToRowTarget = false; + } + // Special case where the previous size was 0: // In this case we don't show any windowed cells at all. // So we should always recalculate offset afterwards. -@@ -594,6 +666,8 @@ var Grid = function (_React$PureComponent) { +@@ -594,6 +674,8 @@ var Grid = function (_React$PureComponent) { if (this._recomputeScrollLeftFlag) { this._recomputeScrollLeftFlag = false; this._updateScrollLeftForScrollToColumn(this.props); @@ -165,7 +173,7 @@ index e1b959a..1e3a269 100644 } else { (0, _updateScrollIndexHelper2.default)({ cellSizeAndPositionManager: instanceProps.columnSizeAndPositionManager, -@@ -616,6 +690,8 @@ var Grid = function (_React$PureComponent) { +@@ -616,6 +698,8 @@ var Grid = function (_React$PureComponent) { if (this._recomputeScrollTopFlag) { this._recomputeScrollTopFlag = false; this._updateScrollTopForScrollToRow(this.props); @@ -174,7 +182,7 @@ index e1b959a..1e3a269 100644 } else { (0, _updateScrollIndexHelper2.default)({ cellSizeAndPositionManager: instanceProps.rowSizeAndPositionManager, -@@ -630,11 +706,56 @@ var Grid = function (_React$PureComponent) { +@@ -630,11 +714,56 @@ var Grid = function (_React$PureComponent) { size: height, sizeJustIncreasedFromZero: sizeJustIncreasedFromZero, updateScrollIndexCallback: function updateScrollIndexCallback() { @@ -232,7 +240,7 @@ index e1b959a..1e3a269 100644 // Update onRowsRendered callback if start/stop indices have changed this._invokeOnGridRenderedHelper(); -@@ -647,7 +768,11 @@ var Grid = function (_React$PureComponent) { +@@ -647,7 +776,11 @@ var Grid = function (_React$PureComponent) { scrollLeft: scrollLeft, scrollTop: scrollTop, totalColumnsWidth: totalColumnsWidth, @@ -245,7 +253,7 @@ index e1b959a..1e3a269 100644 }); } -@@ -962,7 +1087,11 @@ var Grid = function (_React$PureComponent) { +@@ -962,7 +1095,11 @@ var Grid = function (_React$PureComponent) { var scrollLeft = _ref6.scrollLeft, scrollTop = _ref6.scrollTop, totalColumnsWidth = _ref6.totalColumnsWidth, @@ -258,7 +266,7 @@ index e1b959a..1e3a269 100644 this._onScrollMemoizer({ callback: function callback(_ref7) { -@@ -973,19 +1102,26 @@ var Grid = function (_React$PureComponent) { +@@ -973,19 +1110,26 @@ var Grid = function (_React$PureComponent) { onScroll = _props7.onScroll, width = _props7.width; @@ -289,7 +297,7 @@ index e1b959a..1e3a269 100644 }); } diff --git a/node_modules/react-virtualized/dist/commonjs/Grid/accessibilityOverscanIndicesGetter.js b/node_modules/react-virtualized/dist/commonjs/Grid/accessibilityOverscanIndicesGetter.js -index 70b0abe..2f04448 100644 +index 70b0abe..8e12ffc 100644 --- a/node_modules/react-virtualized/dist/commonjs/Grid/accessibilityOverscanIndicesGetter.js +++ b/node_modules/react-virtualized/dist/commonjs/Grid/accessibilityOverscanIndicesGetter.js @@ -32,15 +32,8 @@ function defaultOverscanIndicesGetter(_ref) { @@ -312,8 +320,9 @@ index 70b0abe..2f04448 100644 + overscanStopIndex: Math.min(cellCount - 1, stopIndex + overscanCellsCount), + }; } +\ No newline at end of file diff --git a/node_modules/react-virtualized/dist/commonjs/Grid/defaultOverscanIndicesGetter.js b/node_modules/react-virtualized/dist/commonjs/Grid/defaultOverscanIndicesGetter.js -index d5f6d04..e01e69a 100644 +index d5f6d04..c4b3d84 100644 --- a/node_modules/react-virtualized/dist/commonjs/Grid/defaultOverscanIndicesGetter.js +++ b/node_modules/react-virtualized/dist/commonjs/Grid/defaultOverscanIndicesGetter.js @@ -27,15 +27,8 @@ function defaultOverscanIndicesGetter(_ref) { @@ -336,6 +345,7 @@ index d5f6d04..e01e69a 100644 + overscanStopIndex: Math.min(cellCount - 1, stopIndex + overscanCellsCount), + }; } +\ No newline at end of file diff --git a/node_modules/react-virtualized/dist/commonjs/List/List.js b/node_modules/react-virtualized/dist/commonjs/List/List.js index b5ad0eb..efb2cd7 100644 --- a/node_modules/react-virtualized/dist/commonjs/List/List.js diff --git a/stylesheets/_modules.scss b/stylesheets/_modules.scss index 9e52d4af9..e46988e79 100644 --- a/stylesheets/_modules.scss +++ b/stylesheets/_modules.scss @@ -12,7 +12,8 @@ // can be used outside tht Timeline contact more easily. .module-message-container { width: 100%; - margin-top: 10px; + margin-top: 4px; + margin-bottom: 4px; &::after { visibility: hidden; @@ -65,7 +66,7 @@ } .module-message--incoming.module-message--group { - margin-left: 44px; + margin-left: 48px; } .module-message__buttons { @@ -175,6 +176,14 @@ padding-top: 10px; padding-bottom: 10px; min-width: 0px; + + @include light-theme { + border: 1px solid $color-white; + } + + @include dark-theme { + border: 1px solid $color-gray-95; + } } .module-message__container--with-sticker { @@ -185,34 +194,23 @@ background-color: $color-light-10; } -.module-message__container__selection { - position: absolute; - display: block; - top: 0; - left: 0; - right: 0; - bottom: 0; - - border-radius: 16px; - - background-color: $color-black; - opacity: 0; - - animation: message--selected 1s ease-in-out; +.module-message__container--selected { + // cubic-bezier taken from https://easings.net/en#easeOutExpo + animation: message--selected 1s cubic-bezier(0.19, 1, 0.22, 1); } @keyframes message--selected { 0% { - opacity: 0; + box-shadow: 0 0 0 5px transparent; } - 20% { - opacity: 0.3; + 10% { + box-shadow: 0 0 0 5px $color-signal-blue; } - 80% { - opacity: 0.3; + 70% { + box-shadow: 0 0 0 5px $color-signal-blue; } 100% { - opacity: 0; + box-shadow: 0 0 0 5px transparent; } } @@ -806,7 +804,7 @@ .module-message__author-avatar { position: absolute; bottom: 0px; - right: calc(100% + 4px); + right: calc(100% + 8px); } .module-message__typing-container { @@ -2420,7 +2418,7 @@ } &:focus { - border: solid 1px blue; + border: solid 1px $color-signal-blue; outline: none; } } @@ -2496,7 +2494,6 @@ // Module: Image .module-image { - overflow: hidden; position: relative; display: inline-block; margin: 1px; @@ -2537,6 +2534,18 @@ border-top-left-radius: 10px; } +.module-image--selection--selected { + position: absolute; + top: 1px; + bottom: 1px; + left: 1px; + right: 1px; + border-radius: 10px; + + // cubic-bezier taken from https://easings.net/en#easeOutExpo + animation: message--selected 1s cubic-bezier(0.19, 1, 0.22, 1); +} + .module-image__border-overlay { position: absolute; top: 0; @@ -2551,13 +2560,6 @@ background-color: $color-black-02; } -.module-image__border-overlay--selected { - background-color: $color-black; - opacity: 0; - - animation: message--selected 1s ease-in-out; -} - .module-image__loading-placeholder { display: inline-flex; flex-direction: row; @@ -3504,7 +3506,8 @@ } .module-timeline__message-container { - padding-top: 10px; + padding-top: 4px; + padding-bottom: 4px; } .ReactVirtualized__List { diff --git a/test/spellcheck_test.js b/test/spellcheck_test.js index 57f7b529f..b51daa4e9 100644 --- a/test/spellcheck_test.js +++ b/test/spellcheck_test.js @@ -1,6 +1,15 @@ describe('spellChecker', () => { it('should work', () => { - assert(window.spellChecker.spellCheck('correct')); - assert(!window.spellChecker.spellCheck('fhqwgads')); + let result = null; + + window.spellChecker.spellCheck(['correct'], answer => { + result = answer; + }); + assert.deepEqual(result, []); + + window.spellChecker.spellCheck(['fhqwgads'], answer => { + result = answer; + }); + assert.deepEqual(result, ['fhqwgads']); }); }); diff --git a/ts/components/Lightbox.tsx b/ts/components/Lightbox.tsx index ac45bc77c..d42cd8d94 100644 --- a/ts/components/Lightbox.tsx +++ b/ts/components/Lightbox.tsx @@ -56,6 +56,8 @@ const styles = { paddingLeft: 40, paddingRight: 40, paddingBottom: 0, + // To ensure that a large image doesn't overflow the flex layout + minHeight: '50px', } as React.CSSProperties, objectContainer: { position: 'relative', diff --git a/ts/components/conversation/Image.tsx b/ts/components/conversation/Image.tsx index 454c14f33..7f76c3d42 100644 --- a/ts/components/conversation/Image.tsx +++ b/ts/components/conversation/Image.tsx @@ -90,6 +90,9 @@ export class Image extends React.Component { softCorners ? 'module-image--soft-corners' : null )} > + {isSelected ? ( +
+ ) : null} {pending ? (
{ alt={i18n('imageCaptionIconAlt')} /> ) : null} - {!noBorder || isSelected ? ( + {!noBorder ? (
{ curveBottomRight ? 'module-image--curved-bottom-right' : null, smallCurveTopLeft ? 'module-image--small-curved-top-left' : null, softCorners ? 'module-image--soft-corners' : null, - darkOverlay ? 'module-image__border-overlay--dark' : null, - isSelected ? 'module-image__border-overlay--selected' : null + darkOverlay ? 'module-image__border-overlay--dark' : null )} /> ) : null} diff --git a/ts/components/conversation/Message.tsx b/ts/components/conversation/Message.tsx index 208fcf31c..589fa4370 100644 --- a/ts/components/conversation/Message.tsx +++ b/ts/components/conversation/Message.tsx @@ -1243,17 +1243,6 @@ export class Message extends React.PureComponent { ); } - public renderSelectionHighlight() { - const { isSticker } = this.props; - const { isSelected } = this.state; - - if (!isSelected || isSticker) { - return; - } - - return
; - } - // tslint:disable-next-line cyclomatic-complexity public render() { const { @@ -1270,7 +1259,7 @@ export class Message extends React.PureComponent { isTapToViewError, timestamp, } = this.props; - const { expired, expiring, imageBroken } = this.state; + const { expired, expiring, imageBroken, isSelected } = this.state; const isAttachmentPending = this.isAttachmentPending(); const isButton = isTapToView && !isTapToViewExpired && !isAttachmentPending; @@ -1306,6 +1295,9 @@ export class Message extends React.PureComponent {
{ > {this.renderAuthor()} {this.renderContents()} - {this.renderSelectionHighlight()} {this.renderAvatar()}
{this.renderError(direction === 'outgoing')} diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index 3bb19dffb..3b1ed5c63 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -7484,7 +7484,7 @@ "rule": "React-createRef", "path": "ts/components/Lightbox.js", "line": " this.videoRef = react_1.default.createRef();", - "lineNumber": 183, + "lineNumber": 185, "reasonCategory": "usageTrusted", "updated": "2019-03-09T00:08:44.242Z", "reasonDetail": "Used to auto-start playback on videos" @@ -7493,7 +7493,7 @@ "rule": "React-createRef", "path": "ts/components/Lightbox.tsx", "line": " this.videoRef = React.createRef();", - "lineNumber": 179, + "lineNumber": 181, "reasonCategory": "usageTrusted", "updated": "2019-03-09T00:08:44.242Z", "reasonDetail": "Used to auto-start playback on videos"