Move chunks - UX improvements
- Surround move labels with chip.
- Add move-item icon.
- Remove deprecated dueToMove.
Change-Id: If8d67b9609bee550ebe554bbe32e4fa6e7a425c8
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
index 24d3635..c08764e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified_test.js
@@ -116,7 +116,7 @@
assert.equal(rowEls.length, 3);
assert.isTrue(moveControlsRow.classList.contains('movedOut'));
assert.equal(cells.length, 3);
- assert.isTrue(cells[2].classList.contains('moveDescription'));
+ assert.isTrue(cells[2].classList.contains('moveLabel'));
assert.equal(cells[2].textContent, 'Moved out');
});
@@ -139,7 +139,7 @@
assert.equal(rowEls.length, 3);
assert.isTrue(moveControlsRow.classList.contains('movedIn'));
assert.equal(cells.length, 3);
- assert.isTrue(cells[2].classList.contains('moveDescription'));
+ assert.isTrue(cells[2].classList.contains('moveLabel'));
assert.equal(cells[2].textContent, 'Moved in');
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
index 75f95f3..f628c47 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
@@ -927,13 +927,20 @@
controlsClass = 'movedOut';
descriptionIndex = movedOutIndex;
}
- const controls = document.createElement('tr');
+
+ const controls = this._createElement('tr', `moveControls ${controlsClass}`);
const cells = [...Array(numberOfCells).keys()].map(() =>
- document.createElement('td')
+ this._createElement('td')
);
- controls.classList.add('moveControls', controlsClass);
- cells[descriptionIndex].classList.add('moveDescription');
- cells[descriptionIndex].textContent = descriptionText;
+ const moveDescriptionDiv = this._createElement('div', 'moveDescription');
+ const icon = this._createElement('iron-icon');
+ icon.setAttribute('icon', 'gr-icons:move-item');
+ const span = this._createElement('span', '');
+ span.textContent = descriptionText;
+ moveDescriptionDiv.appendChild(icon);
+ moveDescriptionDiv.appendChild(span);
+ cells[descriptionIndex].appendChild(moveDescriptionDiv);
+ cells[descriptionIndex].classList.add('moveLabel');
cells.forEach(c => {
controls.appendChild(c);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
index 718e11b..987a7d0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
@@ -226,8 +226,7 @@
assert.equal(cursorElement.side, 'left');
});
- // To be removed as soon due_to_move (deprecated) is removed
- suite('moved chunks (dueToMove=true)', () => {
+ suite('moved chunks without line range)', () => {
setup(done => {
const renderHandler = function() {
diffElement.removeEventListener('render', renderHandler);
@@ -247,7 +246,7 @@
'Sagittis tincidunt torquent, tempor nunc amet.',
'At rhoncus id.',
],
- due_to_move: true,
+ move_details: {changed: false},
},
{
ab: [
@@ -260,7 +259,7 @@
'Sagittis tincidunt torquent, tempor nunc amet.',
'At rhoncus id.',
],
- due_to_move: true,
+ move_details: {changed: false},
},
{
ab: [
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
index 77b5499..c3d758c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
@@ -366,8 +366,7 @@
const group = new GrDiffGroup(type, lines);
group.keyLocation = !!chunk.keyLocation;
group.dueToRebase = !!chunk.due_to_rebase;
- group.moveDetails =
- chunk.move_details || (chunk.due_to_move ? {changed: false} : undefined);
+ group.moveDetails = chunk.move_details;
group.skip = chunk.skip;
group.ignoredWhitespaceOnly = !!chunk.common;
if (chunk.skip) {
@@ -698,9 +697,6 @@
if (chunk.due_to_rebase) {
subChunk.due_to_rebase = true;
}
- if (chunk.due_to_move) {
- subChunk.due_to_move = true;
- }
if (chunk.move_details) {
subChunk.move_details = chunk.move_details;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
index ce7a3c4..5496b62 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
@@ -1019,16 +1019,6 @@
}
});
- test('_breakdownChunk keeps due_to_move for broken down additions',
- () => {
- sinon.spy(element, '_breakdown');
- const chunk = {b: ['blah', 'blah', 'blah'], due_to_move: true};
- const result = element._breakdownChunk(chunk);
- for (const subResult of result) {
- assert.isTrue(subResult.due_to_move);
- }
- });
-
test('_breakdown common case', () => {
const array = 'Lorem ipsum dolor sit amet, suspendisse inceptos'
.split(' ');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index f64d940..ae3e016 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -16,6 +16,7 @@
*/
import '../../../styles/shared-styles';
import '../../shared/gr-button/gr-button';
+import '../../shared/gr-icons/gr-icons';
import '../gr-diff-builder/gr-diff-builder-element';
import '../gr-diff-highlight/gr-diff-highlight';
import '../gr-diff-selection/gr-diff-selection';
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
index ee5e1c0..b9578e8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
@@ -211,18 +211,45 @@
/* dueToMove */
.dueToMove .content.add .contentText,
- .dueToMove .moveControls.movedIn .moveDescription,
+ .dueToMove .moveControls.movedIn .moveLabel,
.delta.total.dueToMove .content.add .contentText {
- background-color: var(--light-moved-add-highlight-color);
+ background-color: var(--diff-moved-in-background);
}
+
.dueToMove .content.remove .contentText,
- .dueToMove .moveControls.movedOut .moveDescription,
+ .dueToMove .moveControls.movedOut .moveLabel,
.delta.total.dueToMove .content.remove .contentText {
- background-color: var(--light-remove-add-highlight-color);
+ background-color: var(--diff-moved-out-background);
}
- .moveControls {
- text-align: right;
- font-style: italic;
+
+ .delta.dueToMove .movedIn .moveDescription {
+ color: var(--diff-moved-in-background);
+ background-color: var(--diff-moved-in-label-background);
+ }
+ .delta.dueToMove .movedOut .moveDescription {
+ color: var(--diff-moved-out-background);
+ background-color: var(--diff-moved-out-label-background);
+ }
+ .moveLabel {
+ display: flex;
+ justify-content: flex-end;
+ font-family: var(--header-font-family);
+ }
+ .delta.dueToMove .moveDescription {
+ border-radius: var(--spacing-l);
+ padding: var(--spacing-s) var(--spacing-m);
+ margin: var(--spacing-s);
+ font-weight: 500;
+ line-height: var(--spacing-xl);
+ vertical-align: middle;
+ display: flex;
+ }
+
+ .moveDescription iron-icon {
+ color: inherit;
+ margin-right: var(--spacing-s);
+ height: var(--spacing-xl);
+ width: var(--spacing-xl);
}
/* ignoredWhitespaceOnly */
@@ -284,8 +311,8 @@
background-color: var(--view-background-color);
}
.contextBackground {
- /*
- * One line of background behind the context expanders which they can
+ /*
+ * One line of background behind the context expanders which they can
* render on top of, plus some padding.
*/
height: calc(var(--line-height-normal) + var(--spacing-s));
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
index 7112e3b..1170477 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
@@ -112,6 +112,8 @@
<g id="schedule"><path d="M11.99 2C6.47 2 2 6.48 2 12s4.47 10 9.99 10C17.52 22 22 17.52 22 12S17.52 2 11.99 2zM12 20c-4.42 0-8-3.58-8-8s3.58-8 8-8 8 3.58 8 8-3.58 8-8 8zm.5-13H11v6l5.25 3.15.75-1.23-4.5-2.67z"></path></g>
<!-- This SVG is a copy from material.io https://material.io/icons/#bug_report-->
<g id="bug"><path d="M0 0h24v24H0z" fill="none"/><path d="M20 8h-2.81c-.45-.78-1.07-1.45-1.82-1.96L17 4.41 15.59 3l-2.17 2.17C12.96 5.06 12.49 5 12 5c-.49 0-.96.06-1.41.17L8.41 3 7 4.41l1.62 1.63C7.88 6.55 7.26 7.22 6.81 8H4v2h2.09c-.05.33-.09.66-.09 1v1H4v2h2v1c0 .34.04.67.09 1H4v2h2.81c1.04 1.79 2.97 3 5.19 3s4.15-1.21 5.19-3H20v-2h-2.09c.05-.33.09-.66.09-1v-1h2v-2h-2v-1c0-.34-.04-.67-.09-1H20V8zm-6 8h-4v-2h4v2zm0-4h-4v-2h4v2z"/></g>
+ <!-- This SVG is a copy from material.io https://fonts.gstatic.com/s/i/googlematerialicons/move_item/v1/24px.svg -->
+ <g id="move-item"><path d="M15,19H5V5h10v4h2V5c0-1.1-0.89-2-2-2H5C3.9,3,3,3.9,3,5v14c0,1.1,0.9,2,2,2h10c1.11,0,2-0.9,2-2v-4h-2V19z"/><polygon points="20.01,8.01 18.59,9.41 20.17,11 8,11 8,13 20.17,13 18.59,14.59 20.01,15.99 24,12"/></g>
</defs>
</svg>
</iron-iconset-svg>`;
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index d7b96c8..73dfe5c 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -174,7 +174,10 @@
--diff-trailing-whitespace-indicator: #ff9ad2;
--light-add-highlight-color: #d8fed8;
--light-rebased-add-highlight-color: #eef;
- --light-moved-add-highlight-color: #eef;
+ --diff-moved-in-background: #e4f7fb;
+ --diff-moved-out-background: #f3e8fd;
+ --diff-moved-in-label-background: #007b83;
+ --diff-moved-out-label-background: #681da8;
--light-remove-add-highlight-color: #fff8dc;
--light-remove-highlight-color: #ffebee;
--coverage-covered: #e0f2f1;
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts
index 4d3e6d8..c7c8eb1 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.ts
+++ b/polygerrit-ui/app/styles/themes/dark-theme.ts
@@ -125,7 +125,10 @@
--diff-trailing-whitespace-indicator: #ff9ad2;
--light-add-highlight-color: #0f401f;
--light-rebased-add-highlight-color: #487165;
- --light-moved-add-highlight-color: #487165;
+ --diff-moved-in-background: #006066;
+ --diff-moved-out-background: #681da8;
+ --diff-moved-in-label-background: #cbf0f8;
+ --diff-moved-out-label-background: #e9d2fd;
--light-remove-add-highlight-color: #2f3f2f;
--light-remove-highlight-color: #320404;
--coverage-covered: #112826;
diff --git a/polygerrit-ui/app/types/diff.d.ts b/polygerrit-ui/app/types/diff.d.ts
index b45b461..ed03dcf 100644
--- a/polygerrit-ui/app/types/diff.d.ts
+++ b/polygerrit-ui/app/types/diff.d.ts
@@ -107,25 +107,12 @@
edit_b?: DiffIntralineInfo[];
/** Indicates whether this entry was introduced by a rebase. */
due_to_rebase?: boolean;
- /** @deprecated Use move_details instead. */
- due_to_move?: boolean;
/**
* Provides info about a move operation the chunk.
* It's presence indicates the current chunk exists due to a move.
*/
- move_details?: {
- /** Indicates whether the content of the chunk changes while moving code */
- changed: boolean;
- /**
- * Indicates the range (line numbers) on the other side of the comparison
- * where the code related to the current chunk came from/went to.
- */
- range: {
- start: number;
- end: number;
- };
- };
+ move_details?: MoveDetails;
/**
* Count of lines skipped on both sides when the file is too large to include
* all common lines.
@@ -142,6 +129,22 @@
}
/**
+ * Details about move operation related to a specific chunk.
+ */
+export declare interface MoveDetails {
+ /** Indicates whether the content of the chunk changes while moving code */
+ changed: boolean;
+ /**
+ * Indicates the range (line numbers) on the other side of the comparison
+ * where the code related to the current chunk came from/went to.
+ */
+ range: {
+ start: number;
+ end: number;
+ };
+}
+
+/**
* The DiffWebLinkInfo entity describes a link on a diff screen to an external
* site.
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#diff-web-link-info
