summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJames Lopez <james@jameslopez.es>2018-03-16 15:11:11 +0100
committerJames Lopez <james@jameslopez.es>2018-03-16 15:11:11 +0100
commit575f37b69e409532cc2781e047f859469dccd411 (patch)
tree12991393f753e33100523db951d5a04a6b96a33d
parenta99baf5e1fc8d28ca0637c0cd5b13a54832f866b (diff)
downloadgitlab-ce-575f37b69e409532cc2781e047f859469dccd411.tar.gz
Revert "Merge branch '35475-lazy-diff' into 'master'"
This reverts commit e4fded68248c0989b4c31921f1c9f8103c65aef7.
-rw-r--r--app/assets/javascripts/notes.js68
-rw-r--r--app/controllers/projects/discussions_controller.rb6
-rw-r--r--app/views/discussions/_diff_with_notes.html.haml31
-rw-r--r--app/views/discussions/_discussion.html.haml2
-rw-r--r--changelogs/unreleased/35475-lazy-diff.yml5
-rw-r--r--config/routes/project.rb2
-rw-r--r--spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb1
-rw-r--r--spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb16
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