From c70db1555311ec24eee5129fbdeaaae3993b15f6 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 5 Jan 2023 12:13:27 -0700 Subject: [PATCH 1/7] Update styling of selected line --- app/assets/stylesheets/framework/diffs.scss | 7 +++++-- .../stylesheets/highlight/white_base.scss | 17 ++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index f7cd5d7e183792..2c477c5b7926e9 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -485,13 +485,16 @@ table.code { user-select: none; margin: 0; padding: 0 10px 0 5px; - border-right-width: 1px; - border-right-style: solid; text-align: right; width: 50px; position: relative; white-space: nowrap; + &:nth-of-type(2) { + border-right-width: 1px; + border-right-style: solid; + } + a { transition: none; float: left; diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index 816aa88cfdedd8..c9b3b6c6ecfd7c 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -125,7 +125,7 @@ $white-gc-bg: #eaf2f5; .diff-line-num, .diff-line-num a { - color: $black-transparent; + color: $gray-400; } // Code itself @@ -173,7 +173,7 @@ pre.code, background-color: $line-number-old; a { - color: scale-color($line-number-old, $red: -30%, $green: -30%, $blue: -30%); + color: scale-color($gray-300, $red: -30%, $green: -30%, $blue: -30%); } } @@ -182,7 +182,7 @@ pre.code, background-color: $line-number-new; a { - color: scale-color($line-number-new, $red: -30%, $green: -30%, $blue: -30%); + color: scale-color($gray-200, $red: -30%, $green: -30%, $blue: -30%); } } @@ -192,8 +192,9 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $line-number-select; - border-color: $line-select-yellow-dark; + background-color: $purple-50; + box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; + border-color: $purple-200; } } @@ -247,7 +248,8 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $line-select-yellow; + background-color: $purple-50; + box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; } } @@ -268,7 +270,8 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $line-select-yellow; + background-color: $purple-50; + box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; } } } -- GitLab From a0f6c78c1d7dfa4719a10db938ba5eca052a3a2a Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 9 Jan 2023 11:45:32 -0700 Subject: [PATCH 2/7] Use blue for selected lines --- app/assets/stylesheets/highlight/common.scss | 6 ++- .../stylesheets/highlight/white_base.scss | 43 ++++++++++++++++--- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/highlight/common.scss b/app/assets/stylesheets/highlight/common.scss index 96df8487c0efd4..084f237281c0d1 100644 --- a/app/assets/stylesheets/highlight/common.scss +++ b/app/assets/stylesheets/highlight/common.scss @@ -49,8 +49,8 @@ } @mixin line-number-hover { - background-color: $purple-100; - border-color: $purple-200; + background-color: $blue-100; + border-color: $blue-300; a { color: $gray-600; @@ -75,6 +75,8 @@ .line_holder { .line_content, .line-coverage { + position: relative; + &.conflict_marker_our { background-color: map-get($conflict-colors, #{$theme}-header-head-neutral); border-color: map-get($conflict-colors, #{$theme}-header-head-neutral); diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index c9b3b6c6ecfd7c..96293598c4150b 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -93,6 +93,35 @@ $white-gc-bg: #eaf2f5; } } +@mixin selected-line() { + &::after { + content: ''; + border-top: 1px solid $blue-300; + border-bottom: 1px solid $blue-300; + position: absolute; + top: 0; + left: 0; + display: block; + width: 100%; + height: 100%; + box-sizing: border-box; + } +} + +.commented { + .hll::after { + @include selected-line(); + border-bottom: 0 !important; + } +} + +.commented ~ .commented { + .hll::after { + border-top: 0 !important; + border-bottom: 0 !important; + } +} + // Line numbers .file-line-num { @include line-link($black, 'link'); @@ -192,9 +221,9 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $purple-50; - box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; - border-color: $purple-200; + @include selected-line(); + background-color: $blue-50; + border-color: $blue-300; } } @@ -248,8 +277,8 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $purple-50; - box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; + @include selected-line(); + background-color: $blue-50; } } @@ -270,8 +299,8 @@ pre.code, } &.hll:not(.empty-cell) { - background-color: $purple-50; - box-shadow: inset 0 1px 0 0 $purple-300, inset 0 -1px 0 0 $purple-300; + @include selected-line(); + background-color: $blue-50; } } } -- GitLab From 779676cab41c85aebcb34c6b4856d4fa12ba3d6a Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Tue, 17 Jan 2023 13:17:34 +1100 Subject: [PATCH 3/7] Use highlighted state for start and end Still broken and incomplete --- .../javascripts/diffs/components/diff_row.vue | 21 +++++++++++- .../diffs/components/diff_row_utils.js | 15 +++++++-- .../diffs/components/diff_view.vue | 19 +++++++++-- .../stylesheets/highlight/white_base.scss | 33 ------------------- 4 files changed, 49 insertions(+), 39 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index e5695c4390f165..a8d2b7638398ef 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -60,6 +60,16 @@ export default { type: Boolean, required: true, }, + isFirstHighlightedLine: { + type: Boolean, + required: false, + default: false, + }, + isLastHighlightedLine: { + type: Boolean, + required: false, + default: false, + }, fileLineCoverage: { type: Function, required: true, @@ -81,7 +91,12 @@ export default { ), parallelViewLeftLineType: memoize( (props) => { - return utils.parallelViewLeftLineType(props.line, props.isHighlighted || props.isCommented); + return utils.parallelViewLeftLineType({ + line: props.line, + hll: props.isHighlighted || props.isCommented, + hllStart: props.isFirstHighlightedLine, + hllEnd: props.isLastHighlightedLine, + }); }, (props) => [props.line.left?.type, props.line.right?.type, props.isHighlighted, props.isCommented].join( @@ -120,6 +135,8 @@ export default { return utils.classNameMapCell({ line: props.line.left, hll: props.isHighlighted || props.isCommented, + hllStart: props.isFirstHighlightedLine, + hllEnd: props.isLastHighlightedLine, }); }, (props) => [props.line.left.type, props.isHighlighted, props.isCommented].join(':'), @@ -129,6 +146,8 @@ export default { return utils.classNameMapCell({ line: props.line.right, hll: props.isHighlighted || props.isCommented, + 'gl-border-t gl-border-color-blue-300': props.isFirstHighlightedLine, + 'gl-border-b gl-border-color-blue-300': props.isLastHighlightedLine, }); }, (props) => [props.line.right.type, props.isHighlighted, props.isCommented].join(':'), diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index 479853caae3d62..d6d1bacbc9e96a 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -40,13 +40,15 @@ export const lineCode = (line) => { return line.line_code || line.left?.line_code || line.right?.line_code; }; -export const classNameMapCell = ({ line, hll, isLoggedIn, isHover }) => { +export const classNameMapCell = ({ line, hll, hllStart, hllEnd, isLoggedIn, isHover }) => { if (!line) return []; const { type } = line; return [ type, { + 'gl-border-t gl-border-color-blue-300': hllStart, + 'gl-border-b gl-border-color-blue-300': hllEnd, hll, [LINE_HOVER_CLASS_NAME]: isLoggedIn && isHover && !isContextLine(type) && !isMetaLine(type), old_line: line.type === 'old', @@ -88,14 +90,21 @@ export const addCommentTooltip = (line) => { return tooltip; }; -export const parallelViewLeftLineType = (line, hll) => { +export const parallelViewLeftLineType = ({ line, hll, hllStart, hllEnd }) => { if (line?.right?.type === NEW_NO_NEW_LINE_TYPE) { return OLD_NO_NEW_LINE_TYPE; } const lineTypeClass = line?.left ? line.left.type : EMPTY_CELL_TYPE; - return [lineTypeClass, { hll }]; + return [ + lineTypeClass, + { + hll, + 'gl-border-t gl-border-color-blue-300': hllStart, + 'gl-border-b gl-border-color-blue-300': hllEnd, + }, + ]; }; export const shouldShowCommentButton = (hover, context, meta, discussions) => { diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index aa9a17d18e3c99..90f5909a889cf7 100644 --- a/app/assets/javascripts/diffs/components/diff_view.vue +++ b/app/assets/javascripts/diffs/components/diff_view.vue @@ -59,7 +59,12 @@ export default { }, computed: { ...mapGetters('diffs', ['commitId', 'fileLineCoverage']), - ...mapState('diffs', ['codequalityDiff', 'highlightedRow', 'coverageLoaded']), + ...mapState('diffs', [ + 'codequalityDiff', + 'highlightedRow', + 'coverageLoaded', + 'selectedCommentPosition', + ]), ...mapState({ selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition, selectedCommentPositionHover: ({ notes }) => notes.selectedCommentPositionHover, @@ -144,6 +149,14 @@ export default { false, ); }, + isFirstHighlightedLine(line) { + const lineCode = line.left?.line_code || line.right?.line_code; + return lineCode && lineCode === this.selectedCommentPosition?.start.line_code; + }, + isLastHighlightedLine(line) { + const lineCode = line.left?.line_code || line.right?.line_code; + return lineCode && line.line_code === this.selectedCommentPosition?.end.line_code; + }, handleParallelLineMouseDown(e) { const line = e.target.closest('.diff-td'); if (line) { @@ -230,10 +243,12 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine" + :is-highlighted="isHighlighted(line)" + :is-first-highlighted-line="isFirstHighlightedLine(line)" + :is-last-highlighted-line="isLastHighlightedLine(line)" :inline="inline" :index="index" :code-quality-expanded="codeQualityExpandedLines.includes(getCodeQualityLine(line))" - :is-highlighted="isHighlighted(line)" :file-line-coverage="fileLineCoverage" :coverage-loaded="coverageLoaded" @showCommentForm="(code) => singleLineComment(code, line)" diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index 96293598c4150b..6b400b6794cf5c 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -93,35 +93,6 @@ $white-gc-bg: #eaf2f5; } } -@mixin selected-line() { - &::after { - content: ''; - border-top: 1px solid $blue-300; - border-bottom: 1px solid $blue-300; - position: absolute; - top: 0; - left: 0; - display: block; - width: 100%; - height: 100%; - box-sizing: border-box; - } -} - -.commented { - .hll::after { - @include selected-line(); - border-bottom: 0 !important; - } -} - -.commented ~ .commented { - .hll::after { - border-top: 0 !important; - border-bottom: 0 !important; - } -} - // Line numbers .file-line-num { @include line-link($black, 'link'); @@ -221,9 +192,7 @@ pre.code, } &.hll:not(.empty-cell) { - @include selected-line(); background-color: $blue-50; - border-color: $blue-300; } } @@ -277,7 +246,6 @@ pre.code, } &.hll:not(.empty-cell) { - @include selected-line(); background-color: $blue-50; } } @@ -299,7 +267,6 @@ pre.code, } &.hll:not(.empty-cell) { - @include selected-line(); background-color: $blue-50; } } -- GitLab From de5ba48d5255313e53ff13d20862b9675a73e754 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Tue, 24 Jan 2023 19:55:38 +0400 Subject: [PATCH 4/7] Add border to selected and highlighted diff lines Fix empty diff lines selection --- .../javascripts/diffs/components/diff_row.vue | 92 +++++++++++++------ .../diffs/components/diff_row_utils.js | 31 ++++--- .../diffs/components/diff_view.vue | 2 +- app/assets/stylesheets/framework/diffs.scss | 14 +++ .../stylesheets/highlight/themes/dark.scss | 16 ++-- .../stylesheets/highlight/themes/monokai.scss | 14 +-- .../stylesheets/highlight/themes/none.scss | 4 +- .../highlight/themes/solarized-dark.scss | 14 +-- .../highlight/themes/solarized-light.scss | 16 ++-- .../stylesheets/highlight/white_base.scss | 6 +- app/assets/stylesheets/pages/notes.scss | 1 + 11 files changed, 134 insertions(+), 76 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index a8d2b7638398ef..cbe12850abc1b3 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -94,14 +94,19 @@ export default { return utils.parallelViewLeftLineType({ line: props.line, hll: props.isHighlighted || props.isCommented, - hllStart: props.isFirstHighlightedLine, - hllEnd: props.isLastHighlightedLine, + hllStart: props.isHighlighted || props.isFirstHighlightedLine, + hllEnd: props.isHighlighted || props.isLastHighlightedLine, }); }, (props) => - [props.line.left?.type, props.line.right?.type, props.isHighlighted, props.isCommented].join( - ':', - ), + [ + props.line.left?.type, + props.line.right?.type, + props.isHighlighted, + props.isCommented, + props.isFirstHighlightedLine, + props.isLastHighlightedLine, + ].join(':'), ), coverageStateLeft: memoize( (props) => { @@ -133,24 +138,38 @@ export default { classNameMapCellLeft: memoize( (props) => { return utils.classNameMapCell({ - line: props.line.left, + line: props.line?.left, hll: props.isHighlighted || props.isCommented, - hllStart: props.isFirstHighlightedLine, - hllEnd: props.isLastHighlightedLine, + hllStart: props.isHighlighted || props.isFirstHighlightedLine, + hllEnd: props.isHighlighted || props.isLastHighlightedLine, }); }, - (props) => [props.line.left.type, props.isHighlighted, props.isCommented].join(':'), + (props) => + [ + props.line?.left?.type, + props.isHighlighted, + props.isCommented, + props.isFirstHighlightedLine, + props.isLastHighlightedLine, + ].join(':'), ), classNameMapCellRight: memoize( (props) => { return utils.classNameMapCell({ - line: props.line.right, + line: props.line?.right, hll: props.isHighlighted || props.isCommented, - 'gl-border-t gl-border-color-blue-300': props.isFirstHighlightedLine, - 'gl-border-b gl-border-color-blue-300': props.isLastHighlightedLine, + hllStart: props.isHighlighted || props.isFirstHighlightedLine, + hllEnd: props.isHighlighted || props.isLastHighlightedLine, }); }, - (props) => [props.line.right.type, props.isHighlighted, props.isCommented].join(':'), + (props) => + [ + props.line?.right?.type, + props.isHighlighted, + props.isCommented, + props.isFirstHighlightedLine, + props.isLastHighlightedLine, + ].join(':'), ), shouldRenderCommentButton: memoize( (props) => { @@ -322,15 +341,24 @@ export default { !props.inline || (props.line.left && props.line.left.type === $options.CONFLICT_MARKER) " > -
+
 
-
-
-
+
+
@@ -409,13 +437,13 @@ export default { :class="[ props.line.right.type, $options.coverageStateRight(props).class, - { hll: props.isHighlighted, hll: props.isCommented }, + ...$options.classNameMapCellRight(props), ]" class="diff-td line-coverage right-side has-tooltip" >
-
-
-
+
+
+
+
diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index d6d1bacbc9e96a..887742554eb1cc 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -41,20 +41,23 @@ export const lineCode = (line) => { }; export const classNameMapCell = ({ line, hll, hllStart, hllEnd, isLoggedIn, isHover }) => { - if (!line) return []; - const { type } = line; + const classes = { + 'highlight-top': hllStart, + 'highlight-bottom': hllEnd, + hll, + }; - return [ - type, - { - 'gl-border-t gl-border-color-blue-300': hllStart, - 'gl-border-b gl-border-color-blue-300': hllEnd, - hll, + if (line) { + const { type } = line; + Object.assign(classes, { + [type]: true, [LINE_HOVER_CLASS_NAME]: isLoggedIn && isHover && !isContextLine(type) && !isMetaLine(type), - old_line: line.type === 'old', - new_line: line.type === 'new', - }, - ]; + old_line: type === 'old', + new_line: type === 'new', + }); + } + + return [classes]; }; export const addCommentTooltip = (line) => { @@ -101,8 +104,8 @@ export const parallelViewLeftLineType = ({ line, hll, hllStart, hllEnd }) => { lineTypeClass, { hll, - 'gl-border-t gl-border-color-blue-300': hllStart, - 'gl-border-b gl-border-color-blue-300': hllEnd, + 'highlight-top': hllStart, + 'highlight-bottom': hllEnd, }, ]; }; diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index 90f5909a889cf7..064570c7a6dc6d 100644 --- a/app/assets/javascripts/diffs/components/diff_view.vue +++ b/app/assets/javascripts/diffs/components/diff_view.vue @@ -155,7 +155,7 @@ export default { }, isLastHighlightedLine(line) { const lineCode = line.left?.line_code || line.right?.line_code; - return lineCode && line.line_code === this.selectedCommentPosition?.end.line_code; + return lineCode && lineCode === this.selectedCommentPosition?.end.line_code; }, handleParallelLineMouseDown(e) { const line = e.target.closest('.diff-td'); diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index 2c477c5b7926e9..df4f6118f2055c 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -461,6 +461,20 @@ table.code { table-layout: fixed; border-radius: 0 0 $border-radius-default $border-radius-default; + .diff-td.highlight-top { + box-shadow: 0 -1px $blue-300; + z-index: 1; + } + + .diff-td.highlight-bottom { + box-shadow: 0 1px $blue-300; + z-index: 1; + } + + .diff-td.highlight-top.highlight-bottom { + box-shadow: 0 -1px $blue-300, 0 1px $blue-300; + } + .diff-tr.line_holder .diff-td, tr.line_holder td { line-height: $code-line-height; diff --git a/app/assets/stylesheets/highlight/themes/dark.scss b/app/assets/stylesheets/highlight/themes/dark.scss index 3438a73eff67bf..a5be2da9896132 100644 --- a/app/assets/stylesheets/highlight/themes/dark.scss +++ b/app/assets/stylesheets/highlight/themes/dark.scss @@ -131,7 +131,7 @@ $dark-il: #de935f; @include hljs-override('title\\.class\\.inherited', $dark-no); @include hljs-override('variable\\.constant', $dark-no); @include hljs-override('title\\.function', $dark-nf); - + // Line numbers .file-line-num { @@ -188,13 +188,13 @@ $dark-il: #de935f; @include dark-diff-expansion-line; } - .diff-td.diff-line-num.hll:not(.empty-cell), - .diff-td.line-coverage.hll:not(.empty-cell), - .diff-td.line-codequality.hll:not(.empty-cell), - .diff-td.line_content.hll:not(.empty-cell), - td.diff-line-num.hll:not(.empty-cell), - td.line-coverage.hll:not(.empty-cell), - td.line_content.hll:not(.empty-cell) { + .diff-td.diff-line-num.hll, + .diff-td.line-coverage.hll, + .diff-td.line-codequality.hll, + .diff-td.line_content.hll, + td.diff-line-num.hll, + td.line-coverage.hll, + td.line_content.hll { background-color: $dark-diff-not-empty-bg; border-color: darken($dark-diff-not-empty-bg, 15%); } diff --git a/app/assets/stylesheets/highlight/themes/monokai.scss b/app/assets/stylesheets/highlight/themes/monokai.scss index 02aa9ed6b16ab3..2d09db87c1d8e0 100644 --- a/app/assets/stylesheets/highlight/themes/monokai.scss +++ b/app/assets/stylesheets/highlight/themes/monokai.scss @@ -179,13 +179,13 @@ $monokai-gh: #75715e; @include dark-diff-expansion-line; } - .diff-td.diff-line-num.hll:not(.empty-cell), - .diff-td.line-coverage.hll:not(.empty-cell), - .diff-td.line-codequality.hll:not(.empty-cell), - .diff-td.line_content.hll:not(.empty-cell), - td.diff-line-num.hll:not(.empty-cell), - td.line-coverage.hll:not(.empty-cell), - td.line_content.hll:not(.empty-cell) { + .diff-td.diff-line-num.hll, + .diff-td.line-coverage.hll, + .diff-td.line-codequality.hll, + .diff-td.line_content.hll, + td.diff-line-num.hll, + td.line-coverage.hll, + td.line_content.hll { background-color: $monokai-line-empty-bg; border-color: $monokai-line-empty-border; } diff --git a/app/assets/stylesheets/highlight/themes/none.scss b/app/assets/stylesheets/highlight/themes/none.scss index fa1f7211b3ea2f..4f2e4af4f682d4 100644 --- a/app/assets/stylesheets/highlight/themes/none.scss +++ b/app/assets/stylesheets/highlight/themes/none.scss @@ -120,7 +120,7 @@ @include line-number-hover; } - &.hll:not(.empty-cell) { + &.hll { background-color: $white; border-color: $white-normal; } @@ -175,7 +175,7 @@ @include match-line; } - &.hll:not(.empty-cell) { + &.hll { background-color: $white-normal; } } diff --git a/app/assets/stylesheets/highlight/themes/solarized-dark.scss b/app/assets/stylesheets/highlight/themes/solarized-dark.scss index 18e18619c09686..14194c92daabae 100644 --- a/app/assets/stylesheets/highlight/themes/solarized-dark.scss +++ b/app/assets/stylesheets/highlight/themes/solarized-dark.scss @@ -182,13 +182,13 @@ $solarized-dark-il: #2aa198; @include dark-diff-expansion-line; } - .diff-td.diff-line-num.hll:not(.empty-cell), - .diff-td.line-coverage.hll:not(.empty-cell), - .diff-td.line-codequality.hll:not(.empty-cell), - .diff-td.line_content.hll:not(.empty-cell), - td.diff-line-num.hll:not(.empty-cell), - td.line-coverage.hll:not(.empty-cell), - td.line_content.hll:not(.empty-cell) { + .diff-td.diff-line-num.hll, + .diff-td.line-coverage.hll, + .diff-td.line-codequality.hll, + .diff-td.line_content.hll, + td.diff-line-num.hll, + td.line-coverage.hll, + td.line_content.hll { background-color: $solarized-dark-hll-bg; border-color: darken($solarized-dark-hll-bg, 15%); } diff --git a/app/assets/stylesheets/highlight/themes/solarized-light.scss b/app/assets/stylesheets/highlight/themes/solarized-light.scss index d2dea02440801d..4e244ed7420b3e 100644 --- a/app/assets/stylesheets/highlight/themes/solarized-light.scss +++ b/app/assets/stylesheets/highlight/themes/solarized-light.scss @@ -116,7 +116,7 @@ $solarized-light-il: #2aa198; @include hljs-override('variable\\.constant', $solarized-light-no); @include hljs-override('variable\\.language', $solarized-light-nb); @include hljs-override('params', $solarized-light-nb); - + // Line numbers .file-line-num { @include line-link($black, 'link'); @@ -174,13 +174,13 @@ $solarized-light-il: #2aa198; background-color: $solarized-light-matchline-bg; } - .diff-td.diff-line-num.hll:not(.empty-cell), - .diff-td.line-coverage.hll:not(.empty-cell), - .diff-td.line-codequality.hll:not(.empty-cell), - .diff-td.line_content.hll:not(.empty-cell), - td.diff-line-num.hll:not(.empty-cell), - td.line-coverage.hll:not(.empty-cell), - td.line_content.hll:not(.empty-cell) { + .diff-td.diff-line-num.hll, + .diff-td.line-coverage.hll, + .diff-td.line-codequality.hll, + .diff-td.line_content.hll, + td.diff-line-num.hll, + td.line-coverage.hll, + td.line_content.hll { background-color: $solarized-light-hll-bg; border-color: darken($solarized-light-hll-bg, 15%); } diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index 6b400b6794cf5c..ad64bb5f5aed3d 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -191,7 +191,7 @@ pre.code, @include line-number-hover; } - &.hll:not(.empty-cell) { + &.hll { background-color: $blue-50; } } @@ -245,7 +245,7 @@ pre.code, @include match-line; } - &.hll:not(.empty-cell) { + &.hll { background-color: $blue-50; } } @@ -266,7 +266,7 @@ pre.code, background-color: $line-added; } - &.hll:not(.empty-cell) { + &.hll { background-color: $blue-50; } } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index c6fa1d50997ad9..5d03281a30a7e5 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -671,6 +671,7 @@ $system-note-svg-size: 1rem; } .discussion-reply-holder { + border-top: 0; border-radius: 0 0 $border-radius-default $border-radius-default; position: relative; -- GitLab From d66cc13322265d3ce1151d86548c9d83b66298c9 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Wed, 25 Jan 2023 02:31:22 +0400 Subject: [PATCH 5/7] Use orange color for highlighted lines Fix border color for selected lines --- .../javascripts/diffs/components/diff_row.vue | 21 ++++++------ .../diffs/components/diff_row_utils.js | 32 ++++++++++++++----- .../diffs/components/diff_view.vue | 4 +-- app/assets/stylesheets/framework/diffs.scss | 12 +++++-- app/assets/stylesheets/highlight/common.scss | 3 +- .../stylesheets/highlight/white_base.scss | 9 ++++-- 6 files changed, 54 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_row.vue b/app/assets/javascripts/diffs/components/diff_row.vue index cbe12850abc1b3..dfca6d61270805 100644 --- a/app/assets/javascripts/diffs/components/diff_row.vue +++ b/app/assets/javascripts/diffs/components/diff_row.vue @@ -93,9 +93,10 @@ export default { (props) => { return utils.parallelViewLeftLineType({ line: props.line, - hll: props.isHighlighted || props.isCommented, - hllStart: props.isHighlighted || props.isFirstHighlightedLine, - hllEnd: props.isHighlighted || props.isLastHighlightedLine, + highlighted: props.isHighlighted, + commented: props.isCommented, + selectionStart: props.isFirstHighlightedLine, + selectionEnd: props.isLastHighlightedLine, }); }, (props) => @@ -139,9 +140,10 @@ export default { (props) => { return utils.classNameMapCell({ line: props.line?.left, - hll: props.isHighlighted || props.isCommented, - hllStart: props.isHighlighted || props.isFirstHighlightedLine, - hllEnd: props.isHighlighted || props.isLastHighlightedLine, + highlighted: props.isHighlighted, + commented: props.isCommented, + selectionStart: props.isFirstHighlightedLine, + selectionEnd: props.isLastHighlightedLine, }); }, (props) => @@ -157,9 +159,10 @@ export default { (props) => { return utils.classNameMapCell({ line: props.line?.right, - hll: props.isHighlighted || props.isCommented, - hllStart: props.isHighlighted || props.isFirstHighlightedLine, - hllEnd: props.isHighlighted || props.isLastHighlightedLine, + highlighted: props.isHighlighted, + commented: props.isCommented, + selectionStart: props.isFirstHighlightedLine, + selectionEnd: props.isLastHighlightedLine, }); }, (props) => diff --git a/app/assets/javascripts/diffs/components/diff_row_utils.js b/app/assets/javascripts/diffs/components/diff_row_utils.js index 887742554eb1cc..a489c96b0c9574 100644 --- a/app/assets/javascripts/diffs/components/diff_row_utils.js +++ b/app/assets/javascripts/diffs/components/diff_row_utils.js @@ -40,11 +40,20 @@ export const lineCode = (line) => { return line.line_code || line.left?.line_code || line.right?.line_code; }; -export const classNameMapCell = ({ line, hll, hllStart, hllEnd, isLoggedIn, isHover }) => { +export const classNameMapCell = ({ + line, + highlighted, + commented, + selectionStart, + selectionEnd, + isLoggedIn, + isHover, +}) => { const classes = { - 'highlight-top': hllStart, - 'highlight-bottom': hllEnd, - hll, + 'highlight-top': highlighted || selectionStart, + 'highlight-bottom': highlighted || selectionEnd, + hll: highlighted, + commented, }; if (line) { @@ -93,7 +102,13 @@ export const addCommentTooltip = (line) => { return tooltip; }; -export const parallelViewLeftLineType = ({ line, hll, hllStart, hllEnd }) => { +export const parallelViewLeftLineType = ({ + line, + highlighted, + commented, + selectionStart, + selectionEnd, +}) => { if (line?.right?.type === NEW_NO_NEW_LINE_TYPE) { return OLD_NO_NEW_LINE_TYPE; } @@ -103,9 +118,10 @@ export const parallelViewLeftLineType = ({ line, hll, hllStart, hllEnd }) => { return [ lineTypeClass, { - hll, - 'highlight-top': hllStart, - 'highlight-bottom': hllEnd, + hll: highlighted, + commented, + 'highlight-top': highlighted || selectionStart, + 'highlight-bottom': highlighted || selectionEnd, }, ]; }; diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index 064570c7a6dc6d..45f0cbc9b9423f 100644 --- a/app/assets/javascripts/diffs/components/diff_view.vue +++ b/app/assets/javascripts/diffs/components/diff_view.vue @@ -244,8 +244,8 @@ export default { :is-bottom="index + 1 === diffLinesLength" :is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine" :is-highlighted="isHighlighted(line)" - :is-first-highlighted-line="isFirstHighlightedLine(line)" - :is-last-highlighted-line="isLastHighlightedLine(line)" + :is-first-highlighted-line="isFirstHighlightedLine(line) || index === commentedLines.startLine" + :is-last-highlighted-line="isLastHighlightedLine(line) || index === commentedLines.endLine" :inline="inline" :index="index" :code-quality-expanded="codeQualityExpandedLines.includes(getCodeQualityLine(line))" diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index df4f6118f2055c..16e6e1de76e2ae 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -450,6 +450,11 @@ } } +.code .diff-grid-row.line_holder.diff-tr .diff-td.commented:not(.hll) { + --highlight-border-color: #{$blue-300}; + background-color: $blue-50; +} + .diff-table.code, table.code { width: 100%; @@ -462,17 +467,18 @@ table.code { border-radius: 0 0 $border-radius-default $border-radius-default; .diff-td.highlight-top { - box-shadow: 0 -1px $blue-300; + box-shadow: 0 -1px var(--highlight-border-color, $blue-300); z-index: 1; } .diff-td.highlight-bottom { - box-shadow: 0 1px $blue-300; + box-shadow: 0 1px var(--highlight-border-color, $blue-300); z-index: 1; } .diff-td.highlight-top.highlight-bottom { - box-shadow: 0 -1px $blue-300, 0 1px $blue-300; + box-shadow: 0 -1px var(--highlight-border-color, $blue-300), 0 1px var(--highlight-border-color, $blue-300); + z-index: 2; } .diff-tr.line_holder .diff-td, diff --git a/app/assets/stylesheets/highlight/common.scss b/app/assets/stylesheets/highlight/common.scss index 084f237281c0d1..20f71903697a7d 100644 --- a/app/assets/stylesheets/highlight/common.scss +++ b/app/assets/stylesheets/highlight/common.scss @@ -49,8 +49,7 @@ } @mixin line-number-hover { - background-color: $blue-100; - border-color: $blue-300; + background-color: $purple-100; a { color: $gray-600; diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index ad64bb5f5aed3d..47bce7e09bfe3d 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -192,7 +192,8 @@ pre.code, } &.hll { - background-color: $blue-50; + --highlight-border-color: #{$orange-200}; + background-color: $orange-50; } } @@ -246,7 +247,8 @@ pre.code, } &.hll { - background-color: $blue-50; + --highlight-border-color: #{$orange-200}; + background-color: $orange-50; } } @@ -267,7 +269,8 @@ pre.code, } &.hll { - background-color: $blue-50; + --highlight-border-color: #{$orange-200}; + background-color: $orange-50; } } } -- GitLab From 8637d5e9affeb0837262e1ce3b59704322ae360f Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Wed, 25 Jan 2023 15:55:50 +0400 Subject: [PATCH 6/7] Improve dark themes colors --- .../diffs/components/diff_view.vue | 4 +- app/assets/stylesheets/framework/diffs.scss | 5 + app/assets/stylesheets/highlight/common.scss | 10 + .../stylesheets/highlight/themes/dark.scss | 13 +- .../stylesheets/highlight/themes/monokai.scss | 13 +- .../highlight/themes/solarized-dark.scss | 13 +- .../stylesheets/highlight/themes/white.scss | 2 +- .../stylesheets/highlight/white_base.scss | 2 +- .../diffs/components/diff_row_utils_spec.js | 525 +++++++++--------- 9 files changed, 322 insertions(+), 265 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_view.vue b/app/assets/javascripts/diffs/components/diff_view.vue index 45f0cbc9b9423f..a2e052e0f939dd 100644 --- a/app/assets/javascripts/diffs/components/diff_view.vue +++ b/app/assets/javascripts/diffs/components/diff_view.vue @@ -244,7 +244,9 @@ export default { :is-bottom="index + 1 === diffLinesLength" :is-commented="index >= commentedLines.startLine && index <= commentedLines.endLine" :is-highlighted="isHighlighted(line)" - :is-first-highlighted-line="isFirstHighlightedLine(line) || index === commentedLines.startLine" + :is-first-highlighted-line=" + isFirstHighlightedLine(line) || index === commentedLines.startLine + " :is-last-highlighted-line="isLastHighlightedLine(line) || index === commentedLines.endLine" :inline="inline" :index="index" diff --git a/app/assets/stylesheets/framework/diffs.scss b/app/assets/stylesheets/framework/diffs.scss index 16e6e1de76e2ae..4eb26d533c2cb0 100644 --- a/app/assets/stylesheets/framework/diffs.scss +++ b/app/assets/stylesheets/framework/diffs.scss @@ -453,6 +453,11 @@ .code .diff-grid-row.line_holder.diff-tr .diff-td.commented:not(.hll) { --highlight-border-color: #{$blue-300}; background-color: $blue-50; + + .gl-dark & { + --highlight-border-color: #{$blue-600}; + background-color: $blue-900; + } } .diff-table.code, diff --git a/app/assets/stylesheets/highlight/common.scss b/app/assets/stylesheets/highlight/common.scss index 20f71903697a7d..085e25a0cdc9e3 100644 --- a/app/assets/stylesheets/highlight/common.scss +++ b/app/assets/stylesheets/highlight/common.scss @@ -50,12 +50,22 @@ @mixin line-number-hover { background-color: $purple-100; + border-color: $purple-200; a { color: $gray-600; } } +@mixin line-number-hover-dark { + background-color: $purple-800; + border-color: $purple-300; + + a { + color: $purple-50; + } +} + @mixin conflict-colors($theme) { .diff-line-num { &.conflict_marker_our, diff --git a/app/assets/stylesheets/highlight/themes/dark.scss b/app/assets/stylesheets/highlight/themes/dark.scss index a5be2da9896132..02469cf5165d37 100644 --- a/app/assets/stylesheets/highlight/themes/dark.scss +++ b/app/assets/stylesheets/highlight/themes/dark.scss @@ -174,6 +174,11 @@ $dark-il: #de935f; @include diff-expansion($gray-600, $gray-200, $gray-300, $white); } + .diff-grid-row.line_holder.diff-tr .diff-td.commented:not(.hll) { + --highlight-border-color: #{$blue-600}; + background-color: $blue-900; + } + // Diff line .line_holder { &.match .line_content, @@ -195,8 +200,8 @@ $dark-il: #de935f; td.diff-line-num.hll, td.line-coverage.hll, td.line_content.hll { - background-color: $dark-diff-not-empty-bg; - border-color: darken($dark-diff-not-empty-bg, 15%); + --highlight-border-color: #{$orange-500}; + background-color: $orange-800; } .line-coverage { @@ -239,14 +244,14 @@ $dark-il: #de935f; &:not(.match) .diff-grid-right:hover, &.code-search-line:hover { .diff-line-num:not(.empty-cell) { - @include line-number-hover; + @include line-number-hover-dark; } } .diff-line-num { &.is-over, &.hll:not(.empty-cell).is-over { - @include line-number-hover; + @include line-number-hover-dark; } } diff --git a/app/assets/stylesheets/highlight/themes/monokai.scss b/app/assets/stylesheets/highlight/themes/monokai.scss index 2d09db87c1d8e0..30d04b4002eff5 100644 --- a/app/assets/stylesheets/highlight/themes/monokai.scss +++ b/app/assets/stylesheets/highlight/themes/monokai.scss @@ -148,6 +148,11 @@ $monokai-gh: #75715e; color: $monokai-line-num-color; } + .diff-grid-row.line_holder.diff-tr .diff-td.commented:not(.hll) { + --highlight-border-color: #{$blue-600}; + background-color: $blue-900; + } + // Code itself pre.code, .diff-line-num { @@ -186,8 +191,8 @@ $monokai-gh: #75715e; td.diff-line-num.hll, td.line-coverage.hll, td.line_content.hll { - background-color: $monokai-line-empty-bg; - border-color: $monokai-line-empty-border; + --highlight-border-color: #{$orange-500}; + background-color: $orange-800; } .line-coverage { @@ -230,14 +235,14 @@ $monokai-gh: #75715e; &:not(.match) .diff-grid-right:hover, &.code-search-line:hover { .diff-line-num:not(.empty-cell) { - @include line-number-hover; + @include line-number-hover-dark; } } .diff-line-num { &.is-over, &.hll:not(.empty-cell).is-over { - @include line-number-hover; + @include line-number-hover-dark; } } diff --git a/app/assets/stylesheets/highlight/themes/solarized-dark.scss b/app/assets/stylesheets/highlight/themes/solarized-dark.scss index 14194c92daabae..075510e6e5f0f2 100644 --- a/app/assets/stylesheets/highlight/themes/solarized-dark.scss +++ b/app/assets/stylesheets/highlight/themes/solarized-dark.scss @@ -151,6 +151,11 @@ $solarized-dark-il: #2aa198; color: $solarized-dark-line-color; } + .diff-grid-row.line_holder.diff-tr .diff-td.commented:not(.hll) { + --highlight-border-color: #{$blue-600}; + background-color: $blue-900; + } + // Code itself pre.code, .diff-line-num { @@ -189,8 +194,8 @@ $solarized-dark-il: #2aa198; td.diff-line-num.hll, td.line-coverage.hll, td.line_content.hll { - background-color: $solarized-dark-hll-bg; - border-color: darken($solarized-dark-hll-bg, 15%); + --highlight-border-color: #{$orange-500}; + background-color: $orange-800; } .line-coverage { @@ -201,7 +206,7 @@ $solarized-dark-il: #2aa198; &:not(.match) .diff-grid-right:hover, &.code-search-line:hover { .diff-line-num:not(.empty-cell) { - @include line-number-hover; + @include line-number-hover-dark; } } @@ -240,7 +245,7 @@ $solarized-dark-il: #2aa198; .diff-line-num { &.is-over, &.hll:not(.empty-cell).is-over { - @include line-number-hover; + @include line-number-hover-dark; } } diff --git a/app/assets/stylesheets/highlight/themes/white.scss b/app/assets/stylesheets/highlight/themes/white.scss index f6cce25671fdc2..0a283254a4c0b1 100644 --- a/app/assets/stylesheets/highlight/themes/white.scss +++ b/app/assets/stylesheets/highlight/themes/white.scss @@ -19,4 +19,4 @@ :root { --default-diff-color-deletion: #eb919b; --default-diff-color-addition: #a0f5b4; -} \ No newline at end of file +} diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index 47bce7e09bfe3d..ccb5d96e966ceb 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -131,7 +131,7 @@ $white-gc-bg: #eaf2f5; // Code itself pre.code, .diff-line-num { - border-color: $white-normal; + border-color: rgba(0, 0, 0, 0.1); } &, diff --git a/spec/frontend/diffs/components/diff_row_utils_spec.js b/spec/frontend/diffs/components/diff_row_utils_spec.js index a6f508c73ebb11..6e9eb433924a66 100644 --- a/spec/frontend/diffs/components/diff_row_utils_spec.js +++ b/spec/frontend/diffs/components/diff_row_utils_spec.js @@ -21,262 +21,287 @@ function problemsClone({ }; } -describe('isHighlighted', () => { - it('should return true if line is highlighted', () => { - const line = { line_code: LINE_CODE }; - const isCommented = false; - expect(utils.isHighlighted(LINE_CODE, line, isCommented)).toBe(true); - }); - - it('should return false if line is not highlighted', () => { - const line = { line_code: LINE_CODE }; - const isCommented = false; - expect(utils.isHighlighted('xxx', line, isCommented)).toBe(false); - }); - - it('should return true if isCommented is true', () => { - const line = { line_code: LINE_CODE }; - const isCommented = true; - expect(utils.isHighlighted('xxx', line, isCommented)).toBe(true); - }); -}); - -describe('isContextLine', () => { - it('return true if line type is context', () => { - expect(utils.isContextLine(CONTEXT_LINE_TYPE)).toBe(true); - }); - - it('return false if line type is not context', () => { - expect(utils.isContextLine('xxx')).toBe(false); - }); -}); - -describe('isMatchLine', () => { - it('return true if line type is match', () => { - expect(utils.isMatchLine(MATCH_LINE_TYPE)).toBe(true); - }); - - it('return false if line type is not match', () => { - expect(utils.isMatchLine('xxx')).toBe(false); - }); -}); - -describe('isMetaLine', () => { - it.each` - type | expectation - ${OLD_NO_NEW_LINE_TYPE} | ${true} - ${NEW_NO_NEW_LINE_TYPE} | ${true} - ${EMPTY_CELL_TYPE} | ${true} - ${'xxx'} | ${false} - `('should return $expectation if type is $type', ({ type, expectation }) => { - expect(utils.isMetaLine(type)).toBe(expectation); - }); -}); - -describe('shouldRenderCommentButton', () => { - it('should return false if comment button is not rendered', () => { - expect(utils.shouldRenderCommentButton(true, false)).toBe(false); - }); - - it('should return false if not logged in', () => { - expect(utils.shouldRenderCommentButton(false, true)).toBe(false); - }); - - it('should return true logged in and rendered', () => { - expect(utils.shouldRenderCommentButton(true, true)).toBe(true); - }); -}); - -describe('hasDiscussions', () => { - it('should return false if line is undefined', () => { - expect(utils.hasDiscussions()).toBe(false); - }); - - it('should return false if discussions is undefined', () => { - expect(utils.hasDiscussions({})).toBe(false); - }); - - it('should return false if discussions has legnth of 0', () => { - expect(utils.hasDiscussions({ discussions: [] })).toBe(false); - }); +describe('diff_row_utils', () => { + describe('isHighlighted', () => { + it('should return true if line is highlighted', () => { + const line = { line_code: LINE_CODE }; + const isCommented = false; + expect(utils.isHighlighted(LINE_CODE, line, isCommented)).toBe(true); + }); + + it('should return false if line is not highlighted', () => { + const line = { line_code: LINE_CODE }; + const isCommented = false; + expect(utils.isHighlighted('xxx', line, isCommented)).toBe(false); + }); - it('should return true if discussions has legnth > 0', () => { - expect(utils.hasDiscussions({ discussions: [1] })).toBe(true); - }); -}); - -describe('lineHref', () => { - it(`should return #${LINE_CODE}`, () => { - expect(utils.lineHref({ line_code: LINE_CODE })).toEqual(`#${LINE_CODE}`); - }); - - it(`should return '#' if line is undefined`, () => { - expect(utils.lineHref()).toEqual('#'); - }); - - it(`should return '#' if line_code is undefined`, () => { - expect(utils.lineHref({})).toEqual('#'); - }); -}); - -describe('lineCode', () => { - it(`should return undefined if line_code is undefined`, () => { - expect(utils.lineCode()).toEqual(undefined); - expect(utils.lineCode({ left: {} })).toEqual(undefined); - expect(utils.lineCode({ right: {} })).toEqual(undefined); - }); - - it(`should return ${LINE_CODE}`, () => { - expect(utils.lineCode({ line_code: LINE_CODE })).toEqual(LINE_CODE); - expect(utils.lineCode({ left: { line_code: LINE_CODE } })).toEqual(LINE_CODE); - expect(utils.lineCode({ right: { line_code: LINE_CODE } })).toEqual(LINE_CODE); - }); -}); - -describe('classNameMapCell', () => { - it.each` - line | hll | isLoggedIn | isHover | expectation - ${undefined} | ${true} | ${true} | ${true} | ${[]} - ${{ type: 'new' }} | ${false} | ${false} | ${false} | ${['new', { hll: false, 'is-over': false, new_line: true, old_line: false }]} - ${{ type: 'new' }} | ${true} | ${true} | ${false} | ${['new', { hll: true, 'is-over': false, new_line: true, old_line: false }]} - ${{ type: 'new' }} | ${true} | ${false} | ${true} | ${['new', { hll: true, 'is-over': false, new_line: true, old_line: false }]} - ${{ type: 'new' }} | ${true} | ${true} | ${true} | ${['new', { hll: true, 'is-over': true, new_line: true, old_line: false }]} - `('should return $expectation', ({ line, hll, isLoggedIn, isHover, expectation }) => { - const classes = utils.classNameMapCell({ line, hll, isLoggedIn, isHover }); - expect(classes).toEqual(expectation); - }); -}); - -describe('addCommentTooltip', () => { - const brokenSymLinkTooltip = - 'Commenting on symbolic links that replace or are replaced by files is not supported'; - const brokenRealTooltip = - 'Commenting on files that replace or are replaced by symbolic links is not supported'; - const lineMovedOrRenamedFileTooltip = - 'Commenting on files that are only moved or renamed is not supported'; - const lineWithNoLineCodeTooltip = 'Commenting on this line is not supported'; - const dragTooltip = 'Add a comment to this line or drag for multiple lines'; - - it('should return default tooltip', () => { - expect(utils.addCommentTooltip()).toBeUndefined(); - }); - - it('should return drag comment tooltip when dragging is enabled', () => { - expect(utils.addCommentTooltip({ problems: problemsClone() })).toEqual(dragTooltip); - }); - - it('should return broken symlink tooltip', () => { - expect( - utils.addCommentTooltip({ - problems: problemsClone({ brokenSymlink: { wasSymbolic: true } }), - }), - ).toEqual(brokenSymLinkTooltip); - expect( - utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isSymbolic: true } }) }), - ).toEqual(brokenSymLinkTooltip); - }); - - it('should return broken real tooltip', () => { - expect( - utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { wasReal: true } }) }), - ).toEqual(brokenRealTooltip); - expect( - utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isReal: true } }) }), - ).toEqual(brokenRealTooltip); - }); - - it('reports a tooltip when the line is in a file that has only been moved or renamed', () => { - expect(utils.addCommentTooltip({ problems: problemsClone({ fileOnlyMoved: true }) })).toEqual( - lineMovedOrRenamedFileTooltip, + it('should return true if isCommented is true', () => { + const line = { line_code: LINE_CODE }; + const isCommented = true; + expect(utils.isHighlighted('xxx', line, isCommented)).toBe(true); + }); + }); + + describe('isContextLine', () => { + it('return true if line type is context', () => { + expect(utils.isContextLine(CONTEXT_LINE_TYPE)).toBe(true); + }); + + it('return false if line type is not context', () => { + expect(utils.isContextLine('xxx')).toBe(false); + }); + }); + + describe('isMatchLine', () => { + it('return true if line type is match', () => { + expect(utils.isMatchLine(MATCH_LINE_TYPE)).toBe(true); + }); + + it('return false if line type is not match', () => { + expect(utils.isMatchLine('xxx')).toBe(false); + }); + }); + + describe('isMetaLine', () => { + it.each` + type | expectation + ${OLD_NO_NEW_LINE_TYPE} | ${true} + ${NEW_NO_NEW_LINE_TYPE} | ${true} + ${EMPTY_CELL_TYPE} | ${true} + ${'xxx'} | ${false} + `('should return $expectation if type is $type', ({ type, expectation }) => { + expect(utils.isMetaLine(type)).toBe(expectation); + }); + }); + + describe('shouldRenderCommentButton', () => { + it('should return false if comment button is not rendered', () => { + expect(utils.shouldRenderCommentButton(true, false)).toBe(false); + }); + + it('should return false if not logged in', () => { + expect(utils.shouldRenderCommentButton(false, true)).toBe(false); + }); + + it('should return true logged in and rendered', () => { + expect(utils.shouldRenderCommentButton(true, true)).toBe(true); + }); + }); + + describe('hasDiscussions', () => { + it('should return false if line is undefined', () => { + expect(utils.hasDiscussions()).toBe(false); + }); + + it('should return false if discussions is undefined', () => { + expect(utils.hasDiscussions({})).toBe(false); + }); + + it('should return false if discussions has legnth of 0', () => { + expect(utils.hasDiscussions({ discussions: [] })).toBe(false); + }); + + it('should return true if discussions has legnth > 0', () => { + expect(utils.hasDiscussions({ discussions: [1] })).toBe(true); + }); + }); + + describe('lineHref', () => { + it(`should return #${LINE_CODE}`, () => { + expect(utils.lineHref({ line_code: LINE_CODE })).toEqual(`#${LINE_CODE}`); + }); + + it(`should return '#' if line is undefined`, () => { + expect(utils.lineHref()).toEqual('#'); + }); + + it(`should return '#' if line_code is undefined`, () => { + expect(utils.lineHref({})).toEqual('#'); + }); + }); + + describe('lineCode', () => { + it(`should return undefined if line_code is undefined`, () => { + expect(utils.lineCode()).toEqual(undefined); + expect(utils.lineCode({ left: {} })).toEqual(undefined); + expect(utils.lineCode({ right: {} })).toEqual(undefined); + }); + + it(`should return ${LINE_CODE}`, () => { + expect(utils.lineCode({ line_code: LINE_CODE })).toEqual(LINE_CODE); + expect(utils.lineCode({ left: { line_code: LINE_CODE } })).toEqual(LINE_CODE); + expect(utils.lineCode({ right: { line_code: LINE_CODE } })).toEqual(LINE_CODE); + }); + }); + + describe('classNameMapCell', () => { + it.each` + line | highlighted | commented | selectionStart | selectionEnd | isLoggedIn | isHover | expectation + ${undefined} | ${true} | ${false} | ${false} | ${false} | ${true} | ${true} | ${[{ 'highlight-top': true, 'highlight-bottom': true, hll: true, commented: false }]} + ${undefined} | ${false} | ${true} | ${false} | ${false} | ${true} | ${true} | ${[{ 'highlight-top': false, 'highlight-bottom': false, hll: false, commented: true }]} + ${{ type: 'new' }} | ${false} | ${false} | ${false} | ${false} | ${false} | ${false} | ${[{ new: true, 'highlight-top': false, 'highlight-bottom': false, hll: false, commented: false, 'is-over': false, new_line: true, old_line: false }]} + ${{ type: 'new' }} | ${true} | ${false} | ${false} | ${false} | ${true} | ${false} | ${[{ new: true, 'highlight-top': true, 'highlight-bottom': true, hll: true, commented: false, 'is-over': false, new_line: true, old_line: false }]} + ${{ type: 'new' }} | ${true} | ${false} | ${false} | ${false} | ${false} | ${true} | ${[{ new: true, 'highlight-top': true, 'highlight-bottom': true, hll: true, commented: false, 'is-over': false, new_line: true, old_line: false }]} + ${{ type: 'new' }} | ${true} | ${false} | ${false} | ${false} | ${true} | ${true} | ${[{ new: true, 'highlight-top': true, 'highlight-bottom': true, hll: true, commented: false, 'is-over': true, new_line: true, old_line: false }]} + `( + 'should return $expectation', + ({ + line, + highlighted, + commented, + selectionStart, + selectionEnd, + isLoggedIn, + isHover, + expectation, + }) => { + const classes = utils.classNameMapCell({ + line, + highlighted, + commented, + selectionStart, + selectionEnd, + isLoggedIn, + isHover, + }); + expect(classes).toEqual(expectation); + }, ); }); - it("reports a tooltip when the line doesn't have a line code to leave a comment on", () => { - expect(utils.addCommentTooltip({ problems: problemsClone({ brokenLineCode: true }) })).toEqual( - lineWithNoLineCodeTooltip, + describe('addCommentTooltip', () => { + const brokenSymLinkTooltip = + 'Commenting on symbolic links that replace or are replaced by files is not supported'; + const brokenRealTooltip = + 'Commenting on files that replace or are replaced by symbolic links is not supported'; + const lineMovedOrRenamedFileTooltip = + 'Commenting on files that are only moved or renamed is not supported'; + const lineWithNoLineCodeTooltip = 'Commenting on this line is not supported'; + const dragTooltip = 'Add a comment to this line or drag for multiple lines'; + + it('should return default tooltip', () => { + expect(utils.addCommentTooltip()).toBeUndefined(); + }); + + it('should return drag comment tooltip when dragging is enabled', () => { + expect(utils.addCommentTooltip({ problems: problemsClone() })).toEqual(dragTooltip); + }); + + it('should return broken symlink tooltip', () => { + expect( + utils.addCommentTooltip({ + problems: problemsClone({ brokenSymlink: { wasSymbolic: true } }), + }), + ).toEqual(brokenSymLinkTooltip); + expect( + utils.addCommentTooltip({ + problems: problemsClone({ brokenSymlink: { isSymbolic: true } }), + }), + ).toEqual(brokenSymLinkTooltip); + }); + + it('should return broken real tooltip', () => { + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { wasReal: true } }) }), + ).toEqual(brokenRealTooltip); + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenSymlink: { isReal: true } }) }), + ).toEqual(brokenRealTooltip); + }); + + it('reports a tooltip when the line is in a file that has only been moved or renamed', () => { + expect(utils.addCommentTooltip({ problems: problemsClone({ fileOnlyMoved: true }) })).toEqual( + lineMovedOrRenamedFileTooltip, + ); + }); + + it("reports a tooltip when the line doesn't have a line code to leave a comment on", () => { + expect( + utils.addCommentTooltip({ problems: problemsClone({ brokenLineCode: true }) }), + ).toEqual(lineWithNoLineCodeTooltip); + }); + }); + + describe('parallelViewLeftLineType', () => { + it(`should return ${OLD_NO_NEW_LINE_TYPE}`, () => { + expect( + utils.parallelViewLeftLineType({ line: { right: { type: NEW_NO_NEW_LINE_TYPE } } }), + ).toEqual(OLD_NO_NEW_LINE_TYPE); + }); + + it(`should return 'new'`, () => { + expect(utils.parallelViewLeftLineType({ line: { left: { type: 'new' } } })[0]).toBe('new'); + }); + + it(`should return ${EMPTY_CELL_TYPE}`, () => { + expect(utils.parallelViewLeftLineType({})).toContain(EMPTY_CELL_TYPE); + }); + + it(`should return hll:true`, () => { + expect(utils.parallelViewLeftLineType({ highlighted: true })[1].hll).toBe(true); + }); + }); + + describe('shouldShowCommentButton', () => { + it.each` + hover | context | meta | discussions | expectation + ${true} | ${false} | ${false} | ${false} | ${true} + ${false} | ${false} | ${false} | ${false} | ${false} + ${true} | ${true} | ${false} | ${false} | ${false} + ${true} | ${true} | ${true} | ${false} | ${false} + ${true} | ${true} | ${true} | ${true} | ${false} + `( + 'should return $expectation when hover is $hover', + ({ hover, context, meta, discussions, expectation }) => { + expect(utils.shouldShowCommentButton(hover, context, meta, discussions)).toBe(expectation); + }, ); }); -}); - -describe('parallelViewLeftLineType', () => { - it(`should return ${OLD_NO_NEW_LINE_TYPE}`, () => { - expect(utils.parallelViewLeftLineType({ right: { type: NEW_NO_NEW_LINE_TYPE } })).toEqual( - OLD_NO_NEW_LINE_TYPE, - ); - }); - - it(`should return 'new'`, () => { - expect(utils.parallelViewLeftLineType({ left: { type: 'new' } })).toContain('new'); - }); - - it(`should return ${EMPTY_CELL_TYPE}`, () => { - expect(utils.parallelViewLeftLineType({})).toContain(EMPTY_CELL_TYPE); - }); - - it(`should return hll:true`, () => { - expect(utils.parallelViewLeftLineType({}, true)[1]).toEqual({ hll: true }); - }); -}); - -describe('shouldShowCommentButton', () => { - it.each` - hover | context | meta | discussions | expectation - ${true} | ${false} | ${false} | ${false} | ${true} - ${false} | ${false} | ${false} | ${false} | ${false} - ${true} | ${true} | ${false} | ${false} | ${false} - ${true} | ${true} | ${true} | ${false} | ${false} - ${true} | ${true} | ${true} | ${true} | ${false} - `( - 'should return $expectation when hover is $hover', - ({ hover, context, meta, discussions, expectation }) => { - expect(utils.shouldShowCommentButton(hover, context, meta, discussions)).toBe(expectation); - }, - ); -}); -describe('mapParallel', () => { - it('should assign computed properties to the line object', () => { - const side = { - discussions: [{}], - discussionsExpanded: true, - hasForm: true, - problems: problemsClone(), - }; - const content = { - diffFile: {}, - hasParallelDraftLeft: () => false, - hasParallelDraftRight: () => false, - draftsForLine: () => [], - }; - const line = { left: side, right: side }; - const expectation = { - commentRowClasses: '', - draftRowClasses: 'js-temp-notes-holder', - hasDiscussionsLeft: true, - hasDiscussionsRight: true, - isContextLineLeft: false, - isContextLineRight: false, - isMatchLineLeft: false, - isMatchLineRight: false, - isMetaLineLeft: false, - isMetaLineRight: false, - }; - const leftExpectation = { - renderDiscussion: true, - hasDraft: false, - lineDrafts: [], - hasCommentForm: true, - }; - const rightExpectation = { - renderDiscussion: false, - hasDraft: false, - lineDrafts: [], - hasCommentForm: false, - }; - const mapped = utils.mapParallel(content)(line); - - expect(mapped).toMatchObject(expectation); - expect(mapped.left).toMatchObject(leftExpectation); - expect(mapped.right).toMatchObject(rightExpectation); + describe('mapParallel', () => { + it('should assign computed properties to the line object', () => { + const side = { + discussions: [{}], + discussionsExpanded: true, + hasForm: true, + problems: problemsClone(), + }; + const content = { + diffFile: {}, + hasParallelDraftLeft: () => false, + hasParallelDraftRight: () => false, + draftsForLine: () => [], + }; + const line = { left: side, right: side }; + const expectation = { + commentRowClasses: '', + draftRowClasses: 'js-temp-notes-holder', + hasDiscussionsLeft: true, + hasDiscussionsRight: true, + isContextLineLeft: false, + isContextLineRight: false, + isMatchLineLeft: false, + isMatchLineRight: false, + isMetaLineLeft: false, + isMetaLineRight: false, + }; + const leftExpectation = { + renderDiscussion: true, + hasDraft: false, + lineDrafts: [], + hasCommentForm: true, + }; + const rightExpectation = { + renderDiscussion: false, + hasDraft: false, + lineDrafts: [], + hasCommentForm: false, + }; + const mapped = utils.mapParallel(content)(line); + + expect(mapped).toMatchObject(expectation); + expect(mapped.left).toMatchObject(leftExpectation); + expect(mapped.right).toMatchObject(rightExpectation); + }); }); }); -- GitLab From 8e6b1740a17c712f198cfec0cd5ce793de7cc6e8 Mon Sep 17 00:00:00 2001 From: Stanislav Lashmanov Date: Sat, 11 Feb 2023 14:54:07 +0400 Subject: [PATCH 7/7] Improve styling for the no code highlight theme --- .../stylesheets/highlight/themes/none.scss | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/app/assets/stylesheets/highlight/themes/none.scss b/app/assets/stylesheets/highlight/themes/none.scss index 4f2e4af4f682d4..8339d7eff80a82 100644 --- a/app/assets/stylesheets/highlight/themes/none.scss +++ b/app/assets/stylesheets/highlight/themes/none.scss @@ -55,7 +55,7 @@ &, pre.code, - .line_holder .line_content { + .line_holder .line_content:not(.hll) { background-color: $white; color: $gl-text-color; } @@ -84,8 +84,8 @@ @include line-coverage-border-color($green-500, $orange-500); } - .line-coverage, - .line-codequality { + .line-coverage:not(.hll), + .line-codequality:not(.hll) { &.old, &.new, &.new-nomappinginraw, @@ -119,11 +119,6 @@ &.hll:not(.empty-cell).is-over { @include line-number-hover; } - - &.hll { - background-color: $white; - border-color: $white-normal; - } } &:not(.diff-expanded) + .diff-expanded, @@ -158,7 +153,7 @@ } } - &.new, &.new-nomappinginraw { + &.new:not(.hll), &.new-nomappinginraw:not(.hll) { background-color: $white-normal; &::before { @@ -174,18 +169,9 @@ &.match { @include match-line; } - - &.hll { - background-color: $white-normal; - } } } - // highlight line via anchor - pre .hll { - background-color: $white-normal; - } - // Search result highlight span.highlight_word { background-color: $white-normal; @@ -197,7 +183,10 @@ text-decoration: underline; } - .hll { background-color: $white; } + .hll { + --highlight-border-color: #{$orange-200}; + background-color: $orange-50; + } .gd { color: $gl-text-color; -- GitLab