diff options
author | Phil Hughes <me@iamphill.com> | 2018-04-17 18:22:44 +0300 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2018-04-17 18:22:44 +0300 |
commit | 606ed05edff01824e0f15a37fa23bd9dacc0cee4 (patch) | |
tree | 7571dad887de0d81c74943c571833cad63f60739 | |
parent | 1a420fd25f4b06504b4f5e338c93a76a66a6564a (diff) | |
parent | 8df69aac04f46f17ce11dbda8ac8568394a34412 (diff) |
Merge branch '45271-collpased-diff-loading' into 'master'
Resolve "Collapsed diff spacing wrong when failed to load"
Closes #45271 and #45317
See merge request gitlab-org/gitlab-ce!18351
-rw-r--r-- | app/assets/javascripts/notes.js | 46 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/buttons.scss | 4 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/diff.scss | 5 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/merge_requests.scss | 17 | ||||
-rw-r--r-- | app/views/discussions/_diff_with_notes.html.haml | 7 | ||||
-rw-r--r-- | changelogs/unreleased/45271-collpased-diff-loading.yml | 5 | ||||
-rw-r--r-- | spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb | 17 |
7 files changed, 84 insertions, 17 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index b0573510ff9..2121907dff0 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 { __ } from '~/locale'; import axios from './lib/utils/axios_utils'; import { getLocationHash } from './lib/utils/url_utility'; import Flash from './flash'; @@ -198,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.onClickRetryLazyLoad.bind(this)); + // fetch notes when tab becomes visible this.$wrapperEl.on('visibilitychange', this.visibilityChange); // when issue status changes, we need to refresh data @@ -244,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'); @@ -1431,16 +1433,15 @@ export default class Notes { syntaxHighlight(fileHolder); } - static renderDiffError($container) { - $container.find('.line_content').html( - $(` - <div class="nothing-here-block"> - ${__( - 'Unable to load the diff.', - )} <a class="js-toggle-lazy-diff" href="javascript:void(0)">Try again</a>? - </div> - `), - ); + onClickRetryLazyLoad(e) { + const $retryButton = $(e.currentTarget); + + $retryButton.prop('disabled', true); + + return this.loadLazyDiff(e) + .then(() => { + $retryButton.prop('disabled', false); + }); } loadLazyDiff(e) { @@ -1449,20 +1450,35 @@ export default class Notes { $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'); - axios + 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. + */ + if (url) { + return 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'); }); + } + return Promise.resolve(); } toggleCommitList(e) { diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index 48f20029383..f4f5926e198 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -503,3 +503,7 @@ fieldset[disabled] .btn, @extend %disabled; } } + +.btn-no-padding { + padding: 0; +} diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 7f037582ca0..11052be40a8 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -160,6 +160,11 @@ } } } + + .diff-loading-error-block { + padding: $gl-padding * 2 $gl-padding; + text-align: center; + } } .image { diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 4692d0fb873..66db4917178 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -762,3 +762,20 @@ 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` +// which 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; + box-shadow: none; + opacity: .65; + + &:hover { + color: $file-mode-changed; + text-decoration: none; + } + } +} diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 8680ec2e298..646e89e9bd1 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 @@ -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.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 - else - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff' 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..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 @@ -27,6 +27,23 @@ 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-lazy-load-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) |