From b01fd7ad81524b5f2773ccba4b5789a6074ffb9d Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 12 Apr 2018 17:29:54 +0100 Subject: [PATCH 1/9] Fixes unresolved discussions rendering the error state instead of the diff --- app/assets/javascripts/notes.js | 19 +++++++++++++------ app/assets/stylesheets/framework/buttons.scss | 4 ++++ .../discussions/_diff_with_notes.html.haml | 2 +- .../45271-collpased-diff-loading.yml | 5 +++++ .../user_scrolls_to_note_on_load_spec.rb | 18 ++++++++++++++++++ 5 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/45271-collpased-diff-loading.yml diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index ac70ddb3ff4..184f335809d 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -19,7 +19,7 @@ import AjaxCache from '~/lib/utils/ajax_cache'; import Vue from 'vue'; import syntaxHighlight from '~/syntax_highlight'; import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue'; -import { __ } from '~/locale'; +import { __, sprintf } from '~/locale'; import axios from './lib/utils/axios_utils'; import { getLocationHash } from './lib/utils/url_utility'; import Flash from './flash'; @@ -1434,10 +1434,11 @@ export default class Notes { static renderDiffError($container) { $container.find('.line_content').html( $(` -
- ${__( - 'Unable to load the diff.', - )} Try again? +
+ ${sprintf(__('Unable to load the diff.%{buttonStartTag}Try again%{buttonEndTag}?'), { + buttonStartTag: '' + }, false)}
`), ); @@ -1455,7 +1456,12 @@ export default class Notes { const fileHolder = $container.find('.file-holder'); const url = fileHolder.data('linesPath'); - axios + /** + * We only fetch resolved discussions. + * Unresolved discussions don't have an endpoint being provided. + */ + if (url) { + axios .get(url) .then(({ data }) => { Notes.renderDiffContent($container, data); @@ -1463,6 +1469,7 @@ export default class Notes { .catch(() => { Notes.renderDiffError($container); }); + } } toggleCommitList(e) { diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 6b89387ab5f..9a8fe12e4e8 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -485,3 +485,7 @@ fieldset[disabled] .btn, @extend %disabled; } } + +.btn-no-padding { + padding: 0; +} \ No newline at end of file diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 8680ec2e298..b382bc201b5 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -7,7 +7,7 @@ - unless expanded - diff_data = { lines_path: project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion) } -.diff-file.file-holder{ class: diff_file_class, data: diff_data } +.diff-file.file-holder.js-lazy-load-discussion{ class: diff_file_class, data: diff_data } .js-file-title.file-title.file-title-flex-parent .file-header-content = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false diff --git a/changelogs/unreleased/45271-collpased-diff-loading.yml b/changelogs/unreleased/45271-collpased-diff-loading.yml new file mode 100644 index 00000000000..fdd13a82a4c --- /dev/null +++ b/changelogs/unreleased/45271-collpased-diff-loading.yml @@ -0,0 +1,5 @@ +--- +title: Fixes unresolved discussions rendering the error state instead of the diff +merge_request: +author: +type: fixed diff --git a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb index 565e375600b..be888bf1a55 100644 --- a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb +++ b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb @@ -27,6 +27,24 @@ describe 'Merge request > User scrolls to note on load', :js do expect(fragment_position_top).to be < (page_scroll_y + page_height) end + it 'renders un-collapsed notes with diff' do + page.current_window.resize_to(1000, 1000) + + visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}" + + page.execute_script "window.scrollTo(0,0)" + + note_element = find(fragment_id) + note_container = note_element.ancestor('.js-toggle-container') + + expect(note_element.visible?).to eq true + + page.within note_container do + expect(page).not_to have_selector('.js-error-load-lazy-diff') + end + + end + it 'expands collapsed notes' do visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}" note_element = find(collapsed_fragment_id) From a236b5e6a22cf762354a9bb3460e6bad80ab5b3f Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 13 Apr 2018 09:16:06 +0100 Subject: [PATCH 2/9] Render only one error message per diff Move html to haml file instead of JS --- app/assets/javascripts/notes.js | 27 ++++++++----------- app/assets/stylesheets/framework/buttons.scss | 2 +- .../discussions/_diff_with_notes.html.haml | 5 +++- .../user_scrolls_to_note_on_load_spec.rb | 2 +- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 184f335809d..9f1db70b06e 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1431,31 +1431,21 @@ export default class Notes { syntaxHighlight(fileHolder); } - static renderDiffError($container) { - $container.find('.line_content').html( - $(` -
- ${sprintf(__('Unable to load the diff.%{buttonStartTag}Try again%{buttonEndTag}?'), { - buttonStartTag: '' - }, false)} -
- `), - ); - } - loadLazyDiff(e) { const $container = $(e.currentTarget).closest('.js-toggle-container'); Notes.renderPlaceholderComponent($container); $container.find('.js-toggle-lazy-diff').removeClass('js-toggle-lazy-diff'); - const tableEl = $container.find('tbody'); - if (tableEl.length === 0) return; + const $tableEl = $container.find('tbody'); + if ($tableEl.length === 0) return; const fileHolder = $container.find('.file-holder'); const url = fileHolder.data('linesPath'); + const $errorContainer = $container.find('.js-error-lazy-load-diff'); + const $successContainer = $container.find('.js-success-lazy-load'); + /** * We only fetch resolved discussions. * Unresolved discussions don't have an endpoint being provided. @@ -1464,10 +1454,15 @@ export default class Notes { axios .get(url) .then(({ data }) => { + // Reset state in case last request returned error + $successContainer.removeClass('hidden'); + $errorContainer.addClass('hidden'); + Notes.renderDiffContent($container, data); }) .catch(() => { - Notes.renderDiffError($container); + $successContainer.addClass('hidden'); + $errorContainer.removeClass('hidden'); }); } } diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 9a8fe12e4e8..0be86c36738 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -488,4 +488,4 @@ fieldset[disabled] .btn, .btn-no-padding { padding: 0; -} \ No newline at end of file +} diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index b382bc201b5..c13a128835a 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -28,8 +28,11 @@ %tr.line_holder.line-holder-placeholder %td.old_line.diff-line-num %td.new_line.diff-line-num - %td.line_content + %td.line_content.js-success-lazy-load .js-code-placeholder + %td.line_content.js-error-lazy-load-diff.hidden + - button = button_tag(_("Try again"), class: "btn-link btn-no-padding js-toggle-lazy-diff") + = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button} = render "discussions/diff_discussion", discussions: [discussion], expanded: true - else - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff' diff --git a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb index be888bf1a55..71cd1ed0776 100644 --- a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb +++ b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb @@ -40,7 +40,7 @@ describe 'Merge request > User scrolls to note on load', :js do expect(note_element.visible?).to eq true page.within note_container do - expect(page).not_to have_selector('.js-error-load-lazy-diff') + expect(page).not_to have_selector('.js-error-lazy-load-diff') end end From 5b171cee3ac20ccbb591aa5e2fad852fcc7440b8 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 13 Apr 2018 09:55:47 +0100 Subject: [PATCH 3/9] Remove unused imports --- app/assets/javascripts/notes.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 9f1db70b06e..a00cf0e239a 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -19,7 +19,6 @@ import AjaxCache from '~/lib/utils/ajax_cache'; import Vue from 'vue'; import syntaxHighlight from '~/syntax_highlight'; import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue'; -import { __, sprintf } from '~/locale'; import axios from './lib/utils/axios_utils'; import { getLocationHash } from './lib/utils/url_utility'; import Flash from './flash'; From 96d8a5adf34f394a330684c08873a93bb791e1d3 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 13 Apr 2018 10:16:52 +0100 Subject: [PATCH 4/9] Fix rubocop --- spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb index 71cd1ed0776..3b6fffb7abd 100644 --- a/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb +++ b/spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb @@ -42,7 +42,6 @@ describe 'Merge request > User scrolls to note on load', :js do page.within note_container do expect(page).not_to have_selector('.js-error-lazy-load-diff') end - end it 'expands collapsed notes' do From 8f189df86f6ebd2bd4974ce180ed75e9d97cd81e Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 16 Apr 2018 12:30:46 +0100 Subject: [PATCH 5/9] Use different class for try again button in collapsed diffs --- app/assets/javascripts/notes.js | 3 +++ app/views/discussions/_diff_with_notes.html.haml | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index a00cf0e239a..a4669b1672a 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -197,6 +197,8 @@ export default class Notes { ); this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff); + this.$wrapperEl.on('click', '.js-toggle-lazy-diff-retry-button', this.loadLazyDiff); + // fetch notes when tab becomes visible this.$wrapperEl.on('visibilitychange', this.visibilityChange); // when issue status changes, we need to refresh data @@ -243,6 +245,7 @@ export default class Notes { this.$wrapperEl.off('click', '.js-comment-resolve-button'); this.$wrapperEl.off('click', '.system-note-commit-list-toggler'); this.$wrapperEl.off('click', '.js-toggle-lazy-diff'); + this.$wrapperEl.off('click', '.js-toggle-lazy-diff-retry-button'); this.$wrapperEl.off('ajax:success', '.js-main-target-form'); this.$wrapperEl.off('ajax:success', '.js-discussion-note-form'); this.$wrapperEl.off('ajax:complete', '.js-main-target-form'); diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index c13a128835a..e026e6a83ae 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -31,7 +31,7 @@ %td.line_content.js-success-lazy-load .js-code-placeholder %td.line_content.js-error-lazy-load-diff.hidden - - button = button_tag(_("Try again"), class: "btn-link btn-no-padding js-toggle-lazy-diff") + - button = button_tag(_("Try again"), class: "btn-link btn-no-padding js-toggle-lazy-diff-retry-button") = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button} = render "discussions/diff_discussion", discussions: [discussion], expanded: true - else From 4b2ff003920cff431ef4e3e8b0436aa924b34fda Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 16 Apr 2018 16:36:24 +0100 Subject: [PATCH 6/9] Disable try again button while fetching the API --- app/assets/javascripts/notes.js | 16 +++++++++++++-- .../stylesheets/pages/merge_requests.scss | 20 +++++++++++++++++++ .../discussions/_diff_with_notes.html.haml | 2 +- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index a4669b1672a..a07af3ee93a 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -197,7 +197,7 @@ export default class Notes { ); this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff); - this.$wrapperEl.on('click', '.js-toggle-lazy-diff-retry-button', this.loadLazyDiff); + this.$wrapperEl.on('click', '.js-toggle-lazy-diff-retry-button', this.onClickRetryLazyLoad.bind(this)); // fetch notes when tab becomes visible this.$wrapperEl.on('visibilitychange', this.visibilityChange); @@ -1433,6 +1433,17 @@ export default class Notes { syntaxHighlight(fileHolder); } + onClickRetryLazyLoad(e) { + const $retryButton = $(e.currentTarget); + + $retryButton.prop('disabled', true); + + return this.loadLazyDiff(e) + .then(() => { + $retryButton.prop('disabled', false); + }); + } + loadLazyDiff(e) { const $container = $(e.currentTarget).closest('.js-toggle-container'); Notes.renderPlaceholderComponent($container); @@ -1453,7 +1464,7 @@ export default class Notes { * Unresolved discussions don't have an endpoint being provided. */ if (url) { - axios + return axios .get(url) .then(({ data }) => { // Reset state in case last request returned error @@ -1467,6 +1478,7 @@ export default class Notes { $errorContainer.removeClass('hidden'); }); } + return Promise.resolve(); } toggleCommitList(e) { diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 4692d0fb873..85beba92da4 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -762,3 +762,23 @@ max-width: 100%; } } + +// Hack alert: we've rewritten `btn` class in a way that +// we've broken it and it is not possible to use with `btn-link` +// wich causes a blank button when it's disabled and hovering +// The css in here is the boostrap one +.btn-link-retry { + + &[disabled] { + cursor: not-allowed; + filter: alpha(opacity=65); + -webkit-box-shadow: none; + box-shadow: none; + opacity: .65; + + &:hover { + color: #777; + text-decoration: none; + } + } +} \ No newline at end of file diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index e026e6a83ae..b27d73c803c 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -31,7 +31,7 @@ %td.line_content.js-success-lazy-load .js-code-placeholder %td.line_content.js-error-lazy-load-diff.hidden - - button = button_tag(_("Try again"), class: "btn-link btn-no-padding js-toggle-lazy-diff-retry-button") + - button = button_tag(_("Try again"), class: "btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button") = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button} = render "discussions/diff_discussion", discussions: [discussion], expanded: true - else From e49aaf838a017d576b27d117cc298a99a313c0ac Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 16 Apr 2018 18:48:53 +0100 Subject: [PATCH 7/9] Adds padding to the error block --- app/assets/stylesheets/pages/diff.scss | 4 ++++ app/assets/stylesheets/pages/merge_requests.scss | 3 +-- app/views/discussions/_diff_with_notes.html.haml | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 679f783b1b6..a304e019ef1 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -160,6 +160,10 @@ } } } + + .diff-loading-error-block { + padding: $gl-padding $gl-padding*2; + } } .image { diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 85beba92da4..eec1946d879 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -768,7 +768,6 @@ // wich causes a blank button when it's disabled and hovering // The css in here is the boostrap one .btn-link-retry { - &[disabled] { cursor: not-allowed; filter: alpha(opacity=65); @@ -781,4 +780,4 @@ text-decoration: none; } } -} \ No newline at end of file +} diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index b27d73c803c..646e89e9bd1 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -30,7 +30,7 @@ %td.new_line.diff-line-num %td.line_content.js-success-lazy-load .js-code-placeholder - %td.line_content.js-error-lazy-load-diff.hidden + %td.js-error-lazy-load-diff.hidden.diff-loading-error-block - button = button_tag(_("Try again"), class: "btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button") = _("Unable to load the diff. %{button_try_again}").html_safe % { button_try_again: button} = render "discussions/diff_discussion", discussions: [discussion], expanded: true From 740cbd89a0887e2b423f1830015714059ff76ce8 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 17 Apr 2018 09:07:48 +0100 Subject: [PATCH 8/9] Update error block css --- app/assets/stylesheets/pages/diff.scss | 3 ++- app/assets/stylesheets/pages/merge_requests.scss | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index a304e019ef1..36ca2dd87fa 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -162,7 +162,8 @@ } .diff-loading-error-block { - padding: $gl-padding $gl-padding*2; + padding: $gl-padding * 2 $gl-padding; + text-align: center; } } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index eec1946d879..0a7a47fad23 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -770,13 +770,11 @@ .btn-link-retry { &[disabled] { cursor: not-allowed; - filter: alpha(opacity=65); - -webkit-box-shadow: none; box-shadow: none; opacity: .65; &:hover { - color: #777; + color: $file-mode-changed; text-decoration: none; } } From 8df69aac04f46f17ce11dbda8ac8568394a34412 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 17 Apr 2018 09:51:33 +0100 Subject: [PATCH 9/9] Fix typo on docs --- app/assets/stylesheets/pages/merge_requests.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 0a7a47fad23..66db4917178 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -765,7 +765,7 @@ // Hack alert: we've rewritten `btn` class in a way that // we've broken it and it is not possible to use with `btn-link` -// wich causes a blank button when it's disabled and hovering +// which causes a blank button when it's disabled and hovering // The css in here is the boostrap one .btn-link-retry { &[disabled] {