Use options instead of positional parameters I am planning to add one more condition, which will make relying on position very hard to read. Change-Id: I25b6decbc59911bed60171622cd1a6d8f32513e6
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts index 49f7305..9937ebb 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -177,7 +177,9 @@ moveDown() { if (this._getViewMode() === DiffViewMode.SIDE_BY_SIDE) { - this.$.cursorManager.next((row: Element) => this._rowHasSide(row)); + this.$.cursorManager.next({ + filter: (row: Element) => this._rowHasSide(row), + }); } else { this.$.cursorManager.next(); } @@ -185,7 +187,9 @@ moveUp() { if (this._getViewMode() === DiffViewMode.SIDE_BY_SIDE) { - this.$.cursorManager.previous((row: Element) => this._rowHasSide(row)); + this.$.cursorManager.previous({ + filter: (row: Element) => this._rowHasSide(row), + }); } else { this.$.cursorManager.previous(); } @@ -202,11 +206,12 @@ } moveToNextChunk(clipToTop?: boolean, navigateToNextFile?: boolean) { - const result = this.$.cursorManager.next( - (row: HTMLElement) => this._isFirstRowOfChunk(row), - target => (target?.parentNode as HTMLElement)?.scrollHeight || 0, - clipToTop - ); + const result = this.$.cursorManager.next({ + filter: (row: HTMLElement) => this._isFirstRowOfChunk(row), + getTargetHeight: target => + (target?.parentNode as HTMLElement)?.scrollHeight || 0, + clipToTop, + }); /* * If user presses n on the last diff chunk, show a toast informing user * that pressing n again will navigate them to next unreviewed file. @@ -247,21 +252,23 @@ } moveToPreviousChunk() { - this.$.cursorManager.previous((row: HTMLElement) => - this._isFirstRowOfChunk(row) - ); + this.$.cursorManager.previous({ + filter: (row: HTMLElement) => this._isFirstRowOfChunk(row), + }); this._fixSide(); } moveToNextCommentThread() { - this.$.cursorManager.next((row: HTMLElement) => this._rowHasThread(row)); + this.$.cursorManager.next({ + filter: (row: HTMLElement) => this._rowHasThread(row), + }); this._fixSide(); } moveToPreviousCommentThread() { - this.$.cursorManager.previous((row: HTMLElement) => - this._rowHasThread(row) - ); + this.$.cursorManager.previous({ + filter: (row: HTMLElement) => this._rowHasThread(row), + }); this._fixSide(); } @@ -530,17 +537,13 @@ return actions; } - _getStops() { - return this.diffs.reduce( + _updateStops() { + this.$.cursorManager.stops = this.diffs.reduce( (stops: HTMLElement[], diff) => stops.concat(diff.getCursorStops()), [] ); } - _updateStops() { - this.$.cursorManager.stops = this._getStops(); - } - /** * Setup and tear down on-render listeners for any diffs that are added or * removed from the cursor.
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts index 1efddaf..efc1203 100644 --- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts +++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.ts
@@ -101,27 +101,33 @@ /** * Move the cursor forward. Clipped to the ends of the stop list. * - * @param condition Optional stop condition. If a condition + * @param options.filter Optional stop condition. If a condition * is passed the cursor will continue to move in the specified direction * until the condition is met. - * @param getTargetHeight Optional function to calculate the + * @param options.getTargetHeight Optional function to calculate the * height of the target's 'section'. The height of the target itself is * sometimes different, used by the diff cursor. - * @param clipToTop When none of the next indices match, move + * @param options.clipToTop When none of the next indices match, move * back to first instead of to last. * @return If a move was performed or why not. * @private */ next( - condition?: (stop: HTMLElement) => boolean, - getTargetHeight?: (target: HTMLElement) => number, - clipToTop?: boolean + options: { + filter?: (stop: HTMLElement) => boolean; + getTargetHeight?: (target: HTMLElement) => number; + clipToTop?: boolean; + } = {} ): CursorMoveResult { - return this._moveCursor(1, condition, getTargetHeight, clipToTop); + return this._moveCursor(1, options); } - previous(condition?: (stop: HTMLElement) => boolean): CursorMoveResult { - return this._moveCursor(-1, condition); + previous( + options: { + filter?: (stop: HTMLElement) => boolean; + } = {} + ): CursorMoveResult { + return this._moveCursor(-1, options); } /** @@ -257,22 +263,28 @@ * end of stop list. * * @param delta either -1 or 1. - * @param condition Optional stop condition. If a condition + * @param options.condition Optional stop condition. If a condition * is passed the cursor will continue to move in the specified direction * until the condition is met. - * @param getTargetHeight Optional function to calculate the + * @param options.getTargetHeight Optional function to calculate the * height of the target's 'section'. The height of the target itself is * sometimes different, used by the diff cursor. - * @param clipToTop When none of the next indices match, move + * @param options.clipToTop When none of the next indices match, move * back to first instead of to last. * @return If a move was performed or why not. * @private */ _moveCursor( delta: number, - condition?: (stop: HTMLElement) => boolean, - getTargetHeight?: (target: HTMLElement) => number, - clipToTop?: boolean + { + filter, + getTargetHeight, + clipToTop, + }: { + filter?: (stop: HTMLElement) => boolean; + getTargetHeight?: (target: HTMLElement) => number; + clipToTop?: boolean; + } = {} ): CursorMoveResult { if (!this.stops.length) { this.unsetCursor(); @@ -281,7 +293,7 @@ this._unDecorateTarget(); - const newIndex = this._getNextindex(delta, condition, clipToTop); + const newIndex = this._getNextIndex(delta, {filter, clipToTop}); const newTarget = newIndex !== -1 ? this.stops[newIndex] : null; const clipped = this.index === newIndex; @@ -324,16 +336,18 @@ * Get the next stop index indicated by the delta direction. * * @param delta either -1 or 1. - * @param condition Optional stop condition. - * @param clipToTop When none of the next indices match, move + * @param options.filter Optional stop condition. + * @param options.clipToTop When none of the next indices match, move * back to first instead of to last. * @return the new index. * @private */ - _getNextindex( + _getNextIndex( delta: number, - condition?: (stop: HTMLElement) => boolean, - clipToTop?: boolean + { + filter, + clipToTop, + }: {filter?: (stop: HTMLElement) => boolean; clipToTop?: boolean} = {} ) { if (!this.stops.length) { return -1; @@ -349,14 +363,14 @@ } while ( (delta > 0 || newIndex > 0) && (delta < 0 || newIndex < this.stops.length - 1) && - condition && - !condition(this.stops[newIndex]) + filter && + !filter(this.stops[newIndex]) ); newIndex = Math.max(0, Math.min(this.stops.length - 1, newIndex)); - // If we failed to satisfy the condition: - if (condition && !condition(this.stops[newIndex])) { + // If we failed to satisfy the filter condition: + if (filter && !filter(this.stops[newIndex])) { if (delta < 0 || clipToTop) { return 0; } else if (delta > 0) {
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js index 5c0bb42..bab4dc4 100644 --- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js +++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.js
@@ -178,14 +178,14 @@ assert.isFalse(getTargetHeight.called); // Move the cursor with an optional get target height function. - element._moveCursor(1, null, getTargetHeight); + element._moveCursor(1, {getTargetHeight}); assert.isTrue(getTargetHeight.called); }); test('_moveCursor from for invalid index does not check height', () => { element.stops = []; const getTargetHeight = sinon.stub(); - element._moveCursor(1, () => false, getTargetHeight); + element._moveCursor(1, () => false, {getTargetHeight}); assert.isFalse(getTargetHeight.called); }); @@ -202,7 +202,7 @@ assert.isTrue(scrollStub.called); }); - test('_getNextindex', () => { + test('_getNextIndex', () => { const isLetterB = function(row) { return row.textContent === 'B'; }; @@ -211,23 +211,23 @@ element.setCursor(list.children[0]); // Move forward to meet the next condition. - assert.equal(element._getNextindex(1, isLetterB), 1); + assert.equal(element._getNextIndex(1, {filter: isLetterB}), 1); element.index = 1; // Nothing else meets the condition, should be at last stop. - assert.equal(element._getNextindex(1, isLetterB), 3); + assert.equal(element._getNextIndex(1, {filter: isLetterB}), 3); element.index = 3; // Should stay at last stop if try to proceed. - assert.equal(element._getNextindex(1, isLetterB), 3); + assert.equal(element._getNextIndex(1, {filter: isLetterB}), 3); // Go back to the previous condition met. Should be back at. // stop 1. - assert.equal(element._getNextindex(-1, isLetterB), 1); + assert.equal(element._getNextIndex(-1, {filter: isLetterB}), 1); element.index = 1; // Go back. No more meet the condition. Should be at stop 0. - assert.equal(element._getNextindex(-1, isLetterB), 0); + assert.equal(element._getNextIndex(-1, {filter: isLetterB}), 0); }); test('focusOnMove prop', () => {