Merge branch '35475-lazy-diff' into 'master'
Resolve "Projects::MergeRequestsController#show is slow (implement skeleton loading)" Closes #35475 See merge request gitlab-org/gitlab-ce!15200
This commit is contained in:
commit
1bab4dcffc
|
@ -16,6 +16,10 @@ 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';
|
||||
|
@ -99,6 +103,13 @@ 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) {
|
||||
|
@ -135,6 +146,8 @@ 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
|
||||
|
@ -173,6 +186,7 @@ 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');
|
||||
|
@ -1207,6 +1221,60 @@ 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');
|
||||
|
|
|
@ -19,6 +19,12 @@ 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
|
||||
|
|
|
@ -2,8 +2,12 @@
|
|||
- 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 }
|
||||
.diff-file.file-holder{ 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
|
||||
|
@ -11,17 +15,24 @@
|
|||
- if diff_file.text?
|
||||
.diff-content.code.js-syntax-highlight
|
||||
%table
|
||||
= render partial: "projects/diffs/line",
|
||||
collection: discussion.truncated_diff_lines,
|
||||
as: :line,
|
||||
locals: { diff_file: diff_file,
|
||||
discussions: discussions,
|
||||
discussion_expanded: true,
|
||||
plain: true }
|
||||
- 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
|
||||
- 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 }
|
||||
|
|
|
@ -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" }
|
||||
%button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button", class: ("js-toggle-lazy-diff" unless expanded) }
|
||||
- if expanded
|
||||
= icon("chevron-up")
|
||||
- else
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: lazy load diffs on merge request discussions
|
||||
merge_request:
|
||||
author:
|
||||
type: performance
|
|
@ -130,7 +130,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
|
|||
post :bulk_update
|
||||
end
|
||||
|
||||
resources :discussions, only: [], constraints: { id: /\h{40}/ } do
|
||||
resources :discussions, only: [:show], constraints: { id: /\h{40}/ } do
|
||||
member do
|
||||
post :resolve
|
||||
delete :resolve, action: :unresolve
|
||||
|
|
|
@ -108,6 +108,7 @@ 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
|
||||
|
|
|
@ -5,15 +5,18 @@ 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)
|
||||
visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
|
||||
end
|
||||
|
||||
it 'scrolls down to fragment' do
|
||||
it 'scrolls note into view' do
|
||||
visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
|
||||
|
||||
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)")
|
||||
|
@ -23,4 +26,13 @@ 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
|
||||
|
|
Loading…
Reference in New Issue