diff options
author | James Lopez <james@jameslopez.es> | 2018-03-16 15:11:11 +0100 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2018-03-16 15:11:11 +0100 |
commit | 575f37b69e409532cc2781e047f859469dccd411 (patch) | |
tree | 12991393f753e33100523db951d5a04a6b96a33d | |
parent | a99baf5e1fc8d28ca0637c0cd5b13a54832f866b (diff) | |
download | gitlab-ce-575f37b69e409532cc2781e047f859469dccd411.tar.gz |
Revert "Merge branch '35475-lazy-diff' into 'master'"
This reverts commit e4fded68248c0989b4c31921f1c9f8103c65aef7.
-rw-r--r-- | app/assets/javascripts/notes.js | 68 | ||||
-rw-r--r-- | app/controllers/projects/discussions_controller.rb | 6 | ||||
-rw-r--r-- | app/views/discussions/_diff_with_notes.html.haml | 31 | ||||
-rw-r--r-- | app/views/discussions/_discussion.html.haml | 2 | ||||
-rw-r--r-- | changelogs/unreleased/35475-lazy-diff.yml | 5 | ||||
-rw-r--r-- | config/routes/project.rb | 2 | ||||
-rw-r--r-- | spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb | 1 | ||||
-rw-r--r-- | spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb | 16 |
8 files changed, 14 insertions, 117 deletions
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 6d1b2f452c0..c640003d958 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -16,10 +16,6 @@ import Autosize from 'autosize'; import 'vendor/jquery.caret'; // required by jquery.atwho import 'vendor/jquery.atwho'; 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'; @@ -103,13 +99,6 @@ export default class Notes { $('.note-edit-form').clone() .addClass('mr-note-edit-form').insertAfter('.note-edit-form'); } - - const hash = getLocationHash(); - const $anchor = hash && document.getElementById(hash); - - if ($anchor) { - this.loadLazyDiff({ currentTarget: $anchor }); - } } setViewType(view) { @@ -146,8 +135,6 @@ export default class Notes { this.$wrapperEl.on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm); // toggle commit list this.$wrapperEl.on('click', '.system-note-commit-list-toggler', this.toggleCommitList); - - this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff); // fetch notes when tab becomes visible this.$wrapperEl.on('visibilitychange', this.visibilityChange); // when issue status changes, we need to refresh data @@ -186,7 +173,6 @@ export default class Notes { this.$wrapperEl.off('keydown', '.js-note-text'); 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('ajax:success', '.js-main-target-form'); this.$wrapperEl.off('ajax:success', '.js-discussion-note-form'); this.$wrapperEl.off('ajax:complete', '.js-main-target-form'); @@ -1221,60 +1207,6 @@ export default class Notes { return this.notesCountBadge.text(parseInt(this.notesCountBadge.text(), 10) + updateCount); } - static renderPlaceholderComponent($container) { - const el = $container.find('.js-code-placeholder').get(0); - new Vue({ // eslint-disable-line no-new - el, - components: { - SkeletonLoadingContainer, - }, - render(createElement) { - return createElement('skeleton-loading-container'); - }, - }); - } - - static renderDiffContent($container, data) { - const { discussion_html } = data; - const lines = $(discussion_html).find('.line_holder'); - lines.addClass('fade-in'); - $container.find('tbody').prepend(lines); - const fileHolder = $container.find('.file-holder'); - $container.find('.line-holder-placeholder').remove(); - 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> - `), - ); - } - - 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 fileHolder = $container.find('.file-holder'); - const url = fileHolder.data('linesPath'); - - axios.get(url) - .then(({ data }) => { - Notes.renderDiffContent($container, data); - }) - .catch(() => { - Notes.renderDiffError($container); - }); - } - toggleCommitList(e) { const $element = $(e.currentTarget); const $closestSystemCommitList = $element.siblings('.system-note-commit-list'); diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb index cba9a53dc4b..ee507009e50 100644 --- a/app/controllers/projects/discussions_controller.rb +++ b/app/controllers/projects/discussions_controller.rb @@ -19,12 +19,6 @@ class Projects::DiscussionsController < Projects::ApplicationController render_discussion end - def show - render json: { - discussion_html: view_to_html_string('discussions/_diff_with_notes', discussion: discussion, expanded: true) - } - end - private def render_discussion diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 8680ec2e298..f9bfc01f213 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -2,12 +2,8 @@ - blob = discussion.blob - discussions = { discussion.original_line_code => [discussion] } - diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file' -- diff_data = {} -- expanded = discussion.expanded? || local_assigns.fetch(:expanded, nil) -- 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{ class: diff_file_class } .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 @@ -15,24 +11,17 @@ - if diff_file.text? .diff-content.code.js-syntax-highlight %table - - if expanded - - discussions = { discussion.original_line_code => [discussion] } - = render partial: "projects/diffs/line", - collection: discussion.truncated_diff_lines, - as: :line, - locals: { diff_file: diff_file, - discussions: discussions, - discussion_expanded: true, - plain: true } - - else - %tr.line_holder.line-holder-placeholder - %td.old_line.diff-line-num - %td.new_line.diff-line-num - %td.line_content - .js-code-placeholder - = render "discussions/diff_discussion", discussions: [discussion], expanded: true + = render partial: "projects/diffs/line", + collection: discussion.truncated_diff_lines, + as: :line, + locals: { diff_file: diff_file, + discussions: discussions, + discussion_expanded: true, + plain: true } - else - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff' + = render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false } + .note-container = render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse_class: true } diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index e9589213f80..8b9fa3d6b05 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -8,7 +8,7 @@ .discussion.js-toggle-container{ data: { discussion_id: discussion.id } } .discussion-header .discussion-actions - %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button", class: ("js-toggle-lazy-diff" unless expanded) } + %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" } - if expanded = icon("chevron-up") - else diff --git a/changelogs/unreleased/35475-lazy-diff.yml b/changelogs/unreleased/35475-lazy-diff.yml deleted file mode 100644 index bafa66ebe39..00000000000 --- a/changelogs/unreleased/35475-lazy-diff.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: lazy load diffs on merge request discussions -merge_request: -author: -type: performance diff --git a/config/routes/project.rb b/config/routes/project.rb index 24a87b44fc7..710fe0ec325 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -132,7 +132,7 @@ constraints(ProjectUrlConstrainer.new) do post :bulk_update end - resources :discussions, only: [:show], constraints: { id: /\h{40}/ } do + resources :discussions, only: [], constraints: { id: /\h{40}/ } do member do post :resolve delete :resolve, action: :unresolve diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index b4ad4b64d8e..3e83a549682 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -108,7 +108,6 @@ describe 'Merge request > User resolves diff notes and discussions', :js do it 'shows resolved discussion when toggled' do find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click - expect(page.find(".line-holder-placeholder")).to be_visible expect(page.find(".timeline-content #note_#{note.id}")).to be_visible end end 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..8a834adbf17 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 @@ -5,18 +5,15 @@ describe 'Merge request > User scrolls to note on load', :js do let(:user) { project.creator } let(:merge_request) { create(:merge_request, source_project: project, author: user) } let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) } - let(:resolved_note) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) } let(:fragment_id) { "#note_#{note.id}" } - let(:collapsed_fragment_id) { "#note_#{resolved_note.id}" } before do sign_in(user) page.current_window.resize_to(1000, 300) - end - - it 'scrolls note into view' do visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}" + end + it 'scrolls down to fragment' do page_height = page.current_window.size[1] page_scroll_y = page.evaluate_script("window.scrollY") fragment_position_top = page.evaluate_script("Math.round($('#{fragment_id}').offset().top)") @@ -26,13 +23,4 @@ describe 'Merge request > User scrolls to note on load', :js do expect(fragment_position_top).to be >= page_scroll_y expect(fragment_position_top).to be < (page_scroll_y + page_height) end - - it 'expands collapsed notes' do - visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}" - note_element = find(collapsed_fragment_id) - note_container = note_element.ancestor('.js-toggle-container') - - expect(note_element.visible?).to eq true - expect(note_container.find('.line_content.noteable_line.old', match: :first).visible?).to eq true - end end |