From c0292d96a1a889aaf17a233a10fda95b46d4786a Mon Sep 17 00:00:00 2001 From: Tim Down Date: Fri, 10 Jun 2022 10:42:28 +0100 Subject: [PATCH] Add vertical padding to CM6 editor content to deal with overflowing entries in the review panel (#8321) GitOrigin-RevId: 9f8265dba773008ba8f97053ce66f23dc4b7121f --- .../controllers/ReviewPanelController.js | 5 +- .../directives/reviewPanelSorted.js | 151 +++++++++++------- .../stylesheets/app/editor/review-panel.less | 4 + 3 files changed, 100 insertions(+), 60 deletions(-) diff --git a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js index df1346a67d..0743e72898 100644 --- a/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js +++ b/services/web/frontend/js/ide/review-panel/controllers/ReviewPanelController.js @@ -137,9 +137,10 @@ export default App.controller( $timeout(() => $scope.$broadcast('review-panel:layout')) ) - $scope.$on('review-panel:sizes', (e, sizes) => + $scope.$on('review-panel:sizes', (e, sizes) => { $scope.$broadcast('editor:set-scroll-size', sizes) - ) + dispatchReviewPanelEvent('sizes', sizes) + }) $scope.$watch('project.features.trackChangesVisible', function (visible) { if (visible == null) { diff --git a/services/web/frontend/js/ide/review-panel/directives/reviewPanelSorted.js b/services/web/frontend/js/ide/review-panel/directives/reviewPanelSorted.js index 65b64aa6dd..09ad75ed4c 100644 --- a/services/web/frontend/js/ide/review-panel/directives/reviewPanelSorted.js +++ b/services/web/frontend/js/ide/review-panel/directives/reviewPanelSorted.js @@ -19,6 +19,16 @@ export default App.directive('reviewPanelSorted', $timeout => ({ link(scope, element, attrs) { let previous_focused_entry_index = 0 + const applyEntryVisibility = function (entry) { + const visible = entry.scope.entry.screenPos + if (visible) { + entry.$wrapper_el.removeClass('rp-entry-hidden') + } else { + entry.$wrapper_el.addClass('rp-entry-hidden') + } + return visible + } + const layout = function (animate) { let entry, height, @@ -51,6 +61,7 @@ export default App.directive('reviewPanelSorted', $timeout => ({ const entries = [] for (const el of Array.from(element.find('.rp-entry-wrapper'))) { entry = { + $wrapper_el: $(el), $indicator_el: $(el).find('.rp-entry-indicator'), $box_el: $(el).find('.rp-entry'), $callout_el: $(el).find('.rp-entry-callout'), @@ -83,9 +94,19 @@ export default App.directive('reviewPanelSorted', $timeout => ({ break } } - const entries_after = entries.slice(focused_entry_index + 1) - const entries_before = entries.slice(0, focused_entry_index) + const focused_entry = entries[focused_entry_index] + const focusedEntryVisible = applyEntryVisibility(focused_entry) + + // If the focused entry has no screenPos, we can't position other entries + // relative to it, so we position all other entries as though the focused + // entry is at the top and they all follow it + const entries_after = focusedEntryVisible + ? entries.slice(focused_entry_index + 1) + : [...entries] + const entries_before = focusedEntryVisible + ? entries.slice(0, focused_entry_index) + : [] previous_focused_entry_index = focused_entry_index sl_console.log('focused_entry_index', focused_entry_index) @@ -112,68 +133,84 @@ export default App.directive('reviewPanelSorted', $timeout => ({ } // Put the focused entry as close to where it wants to be as possible - const focused_entry_top = Math.max( - focused_entry.scope.entry.screenPos.y, - TOOLBAR_HEIGHT - ) - focused_entry.$box_el.css({ - top: focused_entry_top, - // The entry element is invisible by default, to avoid flickering when positioning for - // the first time. Here we make sure it becomes visible after having a "top" value. - visibility: 'visible', - }) - focused_entry.$indicator_el.css({ top: focused_entry_top }) - // use screenPos.height if set - screenPosHeight = - focused_entry.scope.entry.screenPos.height ?? line_height - positionLayoutEl( - focused_entry.$callout_el, - focused_entry.scope.entry.screenPos.y, - focused_entry_top, - screenPosHeight - ) + let focused_entry_top = 0 + let previousBottom = 0 - let previousBottom = focused_entry_top + focused_entry.$layout_el.height() - for (entry of Array.from(entries_after)) { - original_top = entry.scope.entry.screenPos.y - // use screenPos.height if set - screenPosHeight = entry.scope.entry.screenPos.height ?? line_height - ;({ height } = entry) - top = Math.max(original_top, previousBottom + PADDING) - previousBottom = top + height - entry.$box_el.css({ - top, + if (focusedEntryVisible) { + const focusedEntryScreenPos = focused_entry.scope.entry.screenPos + focused_entry_top = Math.max(focusedEntryScreenPos.y, TOOLBAR_HEIGHT) + focused_entry.$box_el.css({ + top: focused_entry_top, // The entry element is invisible by default, to avoid flickering when positioning for // the first time. Here we make sure it becomes visible after having a "top" value. visibility: 'visible', }) - entry.$indicator_el.css({ top }) - positionLayoutEl(entry.$callout_el, original_top, top, screenPosHeight) + focused_entry.$indicator_el.css({ top: focused_entry_top }) + // use screenPos.height and screenPos.editorPaddingTop if set + screenPosHeight = focusedEntryScreenPos.height ?? line_height + positionLayoutEl( + focused_entry.$callout_el, + focusedEntryScreenPos.y + + (focusedEntryScreenPos.editorPaddingTop ?? 0), + focused_entry_top, + screenPosHeight + ) + previousBottom = focused_entry_top + focused_entry.$layout_el.height() + } + + for (entry of entries_after) { + const entryVisible = applyEntryVisibility(entry) + if (entryVisible) { + original_top = entry.scope.entry.screenPos.y + // use screenPos.height if set + screenPosHeight = entry.scope.entry.screenPos.height ?? line_height + ;({ height } = entry) + top = Math.max(original_top, previousBottom + PADDING) + previousBottom = top + height + entry.$box_el.css({ + top, + // The entry element is invisible by default, to avoid flickering when positioning for + // the first time. Here we make sure it becomes visible after having a "top" value. + visibility: 'visible', + }) + entry.$indicator_el.css({ top }) + positionLayoutEl( + entry.$callout_el, + original_top, + top, + screenPosHeight + ) + } sl_console.log('ENTRY', { entry: entry.scope.entry, top }) } - const lastBottom = previousBottom - let previousTop = focused_entry_top entries_before.reverse() // Work through backwards, starting with the one just above - for (i = 0; i < entries_before.length; i++) { - entry = entries_before[i] - original_top = entry.scope.entry.screenPos.y - // use screenPos.height if set - screenPosHeight = entry.scope.entry.screenPos.height ?? line_height - ;({ height } = entry) - const original_bottom = original_top + height - const bottom = Math.min(original_bottom, previousTop - PADDING) - top = bottom - height - previousTop = top - entry.$box_el.css({ - top, - // The entry element is invisible by default, to avoid flickering when positioning for - // the first time. Here we make sure it becomes visible after having a "top" value. - visibility: 'visible', - }) - entry.$indicator_el.css({ top }) - positionLayoutEl(entry.$callout_el, original_top, top, screenPosHeight) + for (entry of entries_before) { + const entryVisible = applyEntryVisibility(entry) + if (entryVisible) { + original_top = entry.scope.entry.screenPos.y + // use screenPos.height if set + screenPosHeight = entry.scope.entry.screenPos.height ?? line_height + ;({ height } = entry) + const original_bottom = original_top + height + const bottom = Math.min(original_bottom, previousTop - PADDING) + top = bottom - height + previousTop = top + entry.$box_el.css({ + top, + // The entry element is invisible by default, to avoid flickering when positioning for + // the first time. Here we make sure it becomes visible after having a "top" value. + visibility: 'visible', + }) + entry.$indicator_el.css({ top }) + positionLayoutEl( + entry.$callout_el, + original_top, + top, + screenPosHeight + ) + } sl_console.log('ENTRY', { entry: entry.scope.entry, top }) } @@ -231,8 +268,6 @@ export default App.directive('reviewPanelSorted', $timeout => ({ if (ignoreNextAceEvent) { return (ignoreNextAceEvent = false) } else { - // TODO: unused? - const ignoreNextPanelEvent = true list.height(height) // console.log({height, scrollTop, top: height - scrollTop}) return list.css({ top: -scrollTop }) @@ -247,9 +282,9 @@ export default App.directive('reviewPanelSorted', $timeout => ({ // receive the scroll position from the CodeMirror 6 track changes extension window.addEventListener('editor:scroll', event => { - const { scrollTop, height } = event.detail + const { scrollTop, height, paddingTop } = event.detail - scrollPanel(scrollTop, height) + scrollPanel(scrollTop - paddingTop, height) }) // send the scroll position to the CodeMirror 6 track changes extension diff --git a/services/web/frontend/stylesheets/app/editor/review-panel.less b/services/web/frontend/stylesheets/app/editor/review-panel.less index 4ec40563e3..41eb763e53 100644 --- a/services/web/frontend/stylesheets/app/editor/review-panel.less +++ b/services/web/frontend/stylesheets/app/editor/review-panel.less @@ -249,6 +249,10 @@ &:hover .rp-entry-comment { display: block; } + + &.rp-entry-hidden { + display: none; + } } .rp-entry {