diff options
author | Sam Bigelow <sbigelow@gitlab.com> | 2019-01-29 22:21:26 -0500 |
---|---|---|
committer | Sam Bigelow <sbigelow@gitlab.com> | 2019-02-06 11:11:09 -0600 |
commit | 2a2f48a5b16d9eaff38317f13aee739cd2c16eaf (patch) | |
tree | a6e347c23193e41510a27287815a9ff8117287c5 | |
parent | e69d9db5d08118cdea27346c314a2a79c56dc9b7 (diff) | |
download | gitlab-ce-2a2f48a5b16d9eaff38317f13aee739cd2c16eaf.tar.gz |
Clearly display diff statistics for MRs52347-lines-changed-statistics-is-not-easily-visible-in-mr-changes-view
Put the statistics in the compare-versions header for the entire MR
Put them in the file header for each individual file
14 files changed, 175 insertions, 51 deletions
diff --git a/app/assets/javascripts/diffs/components/compare_versions.vue b/app/assets/javascripts/diffs/components/compare_versions.vue index 3ef54752436..0bf2dde8b96 100644 --- a/app/assets/javascripts/diffs/components/compare_versions.vue +++ b/app/assets/javascripts/diffs/components/compare_versions.vue @@ -6,6 +6,7 @@ import { polyfillSticky } from '~/lib/utils/sticky'; import Icon from '~/vue_shared/components/icon.vue'; import CompareVersionsDropdown from './compare_versions_dropdown.vue'; import SettingsDropdown from './settings_dropdown.vue'; +import DiffStats from './diff_stats.vue'; export default { components: { @@ -14,6 +15,7 @@ export default { GlLink, GlButton, SettingsDropdown, + DiffStats, }, directives: { GlTooltip: GlTooltipDirective, @@ -35,8 +37,15 @@ export default { }, }, computed: { - ...mapState('diffs', ['commit', 'showTreeList', 'startVersion', 'latestVersionPath']), - ...mapGetters('diffs', ['hasCollapsedFile']), + ...mapGetters('diffs', ['hasCollapsedFile', 'diffFilesLength']), + ...mapState('diffs', [ + 'commit', + 'showTreeList', + 'startVersion', + 'latestVersionPath', + 'addedLines', + 'removedLines', + ]), comparableDiffs() { return this.mergeRequestDiffs.slice(1); }, @@ -104,6 +113,11 @@ export default { <gl-link :href="commit.commit_url" class="monospace">{{ commit.short_id }}</gl-link> </div> <div class="inline-parallel-buttons d-none d-md-flex ml-auto"> + <diff-stats + :diff-files-length="diffFilesLength" + :added-lines="addedLines" + :removed-lines="removedLines" + /> <gl-button v-if="commit || startVersion" :href="latestVersionPath" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index b58f704bebb..60586d4a607 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -9,6 +9,7 @@ import { GlTooltipDirective } from '@gitlab/ui'; import { truncateSha } from '~/lib/utils/text_utility'; import { __, s__, sprintf } from '~/locale'; import EditButton from './edit_button.vue'; +import DiffStats from './diff_stats.vue'; export default { components: { @@ -16,6 +17,7 @@ export default { EditButton, Icon, FileIcon, + DiffStats, }, directives: { GlTooltip: GlTooltipDirective, @@ -202,6 +204,7 @@ export default { v-if="!diffFile.submodule && addMergeRequestButtons" class="file-actions d-none d-sm-block" > + <diff-stats :added-lines="diffFile.added_lines" :removed-lines="diffFile.removed_lines" /> <template v-if="diffFile.blob && diffFile.blob.readable_text"> <button :disabled="!diffHasDiscussions(diffFile)" diff --git a/app/assets/javascripts/diffs/components/diff_stats.vue b/app/assets/javascripts/diffs/components/diff_stats.vue new file mode 100644 index 00000000000..2e5855380af --- /dev/null +++ b/app/assets/javascripts/diffs/components/diff_stats.vue @@ -0,0 +1,52 @@ +<script> +import Icon from '~/vue_shared/components/icon.vue'; +import { n__ } from '~/locale'; + +export default { + components: { Icon }, + props: { + addedLines: { + type: Number, + required: true, + }, + removedLines: { + type: Number, + required: true, + }, + diffFilesLength: { + type: Number, + required: false, + default: null, + }, + }, + computed: { + filesText() { + return n__('File', 'Files', this.diffFilesLength); + }, + isCompareVersionsHeader() { + return Boolean(this.diffFilesLength); + }, + }, +}; +</script> + +<template> + <div + class="diff-stats" + :class="{ + 'is-compare-versions-header d-none d-lg-inline-flex': isCompareVersionsHeader, + 'd-inline-flex': !isCompareVersionsHeader, + }" + > + <div v-if="diffFilesLength !== null" class="diff-stats-group"> + <icon name="doc-code" class="diff-stats-icon text-secondary" /> + <strong>{{ diffFilesLength }} {{ filesText }}</strong> + </div> + <div class="diff-stats-group cgreen"> + <icon name="file-addition" class="diff-stats-icon" /> <strong>{{ addedLines }}</strong> + </div> + <div class="diff-stats-group cred"> + <icon name="file-deletion" class="diff-stats-icon" /> <strong>{{ removedLines }}</strong> + </div> + </div> +</template> diff --git a/app/assets/javascripts/diffs/components/tree_list.vue b/app/assets/javascripts/diffs/components/tree_list.vue index 0b3def3d29d..5918527266f 100644 --- a/app/assets/javascripts/diffs/components/tree_list.vue +++ b/app/assets/javascripts/diffs/components/tree_list.vue @@ -19,8 +19,8 @@ export default { }; }, computed: { - ...mapState('diffs', ['tree', 'addedLines', 'removedLines', 'renderTreeList']), - ...mapGetters('diffs', ['allBlobs', 'diffFilesLength']), + ...mapState('diffs', ['tree', 'renderTreeList']), + ...mapGetters('diffs', ['allBlobs']), filteredTreeList() { const search = this.search.toLowerCase().trim(); @@ -90,13 +90,6 @@ export default { {{ s__('MergeRequest|No files found') }} </p> </div> - <div v-once class="pt-3 pb-3 text-center"> - {{ n__('%d changed file', '%d changed files', diffFilesLength) }} - <div> - <span class="cgreen"> {{ n__('%d addition', '%d additions', addedLines) }} </span> - <span class="cred"> {{ n__('%d deleted', '%d deletions', removedLines) }} </span> - </div> - </div> </div> </template> diff --git a/app/assets/javascripts/diffs/store/modules/diff_state.js b/app/assets/javascripts/diffs/store/modules/diff_state.js index 05b4c552f6e..0e3c668fff6 100644 --- a/app/assets/javascripts/diffs/store/modules/diff_state.js +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -11,6 +11,8 @@ const storedTreeShow = localStorage.getItem(MR_TREE_SHOW_KEY); export default () => ({ isLoading: true, + addedLines: null, + removedLines: null, endpoint: '', basePath: '', commit: null, diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 037a5adfb7e..d59bfecd059 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -51,6 +51,7 @@ position: absolute; top: 5px; right: 15px; + margin-left: auto; .btn { padding: 0 10px; @@ -324,6 +325,7 @@ span.idiff { &, .file-holder & { display: flex; + flex-wrap: wrap; align-items: center; justify-content: space-between; background-color: $gray-light; @@ -365,16 +367,12 @@ span.idiff { margin: 0 10px 0 0; } - .file-actions { - white-space: nowrap; - - .btn { - padding: 0 10px; - font-size: 13px; - line-height: 28px; - display: inline-block; - float: none; - } + .file-actions .btn { + padding: 0 10px; + font-size: 13px; + line-height: 28px; + display: inline-block; + float: none; } @include media-breakpoint-down(xs) { diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 02aac58a475..39793337971 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -501,6 +501,25 @@ } } +.diff-stats { + align-items: center; + padding: 0 .25rem; + + .diff-stats-group { + padding: 0 .25rem; + } + + svg.diff-stats-icon { + vertical-align: text-bottom; + } + + &.is-compare-versions-header { + .diff-stats-group { + padding: 0 .5rem; + } + } +} + .file-content .diff-file { margin: 0; border: 0; diff --git a/changelogs/unreleased/52347-lines-changed-statistics-is-not-easily-visible-in-mr-changes-view.yml b/changelogs/unreleased/52347-lines-changed-statistics-is-not-easily-visible-in-mr-changes-view.yml new file mode 100644 index 00000000000..cf1c4378f18 --- /dev/null +++ b/changelogs/unreleased/52347-lines-changed-statistics-is-not-easily-visible-in-mr-changes-view.yml @@ -0,0 +1,5 @@ +--- +title: Show MR statistics in diff comparisons +merge_request: !24569 +author: +type: changed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index ccdbc63c51e..c27ebbf567e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -22,16 +22,6 @@ msgstr "" msgid " or " msgstr "" -msgid "%d addition" -msgid_plural "%d additions" -msgstr[0] "" -msgstr[1] "" - -msgid "%d changed file" -msgid_plural "%d changed files" -msgstr[0] "" -msgstr[1] "" - msgid "%d commit" msgid_plural "%d commits" msgstr[0] "" @@ -42,11 +32,6 @@ msgid_plural "%d commits behind" msgstr[0] "" msgstr[1] "" -msgid "%d deleted" -msgid_plural "%d deletions" -msgstr[0] "" -msgstr[1] "" - msgid "%d exporter" msgid_plural "%d exporters" msgstr[0] "" @@ -3214,6 +3199,11 @@ msgstr "" msgid "Fields on this page are now uneditable, you can configure" msgstr "" +msgid "File" +msgid_plural "Files" +msgstr[0] "" +msgstr[1] "" + msgid "File added" msgstr "" diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb index 63d8decc2d2..aa91ade46ca 100644 --- a/spec/features/merge_request/user_sees_versions_spec.rb +++ b/spec/features/merge_request/user_sees_versions_spec.rb @@ -42,7 +42,7 @@ describe 'Merge request > User sees versions', :js do expect(page).to have_content 'latest version' end - expect(page).to have_content '8 changed files' + expect(page).to have_content '8 Files' end it_behaves_like 'allows commenting', @@ -76,7 +76,7 @@ describe 'Merge request > User sees versions', :js do end it 'shows comments that were last relevant at that version' do - expect(page).to have_content '5 changed files' + expect(page).to have_content '5 Files' position = Gitlab::Diff::Position.new( old_path: ".gitmodules", @@ -120,8 +120,15 @@ describe 'Merge request > User sees versions', :js do diff_id: merge_request_diff3.id, start_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' ) - expect(page).to have_content '4 changed files' - expect(page).to have_content '15 additions 6 deletions' + expect(page).to have_content '4 Files' + + additions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-addition') + .ancestor('.diff-stats-group').text + deletions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-deletion') + .ancestor('.diff-stats-group').text + + expect(additions_content).to eq '15' + expect(deletions_content).to eq '6' position = Gitlab::Diff::Position.new( old_path: ".gitmodules", @@ -141,8 +148,14 @@ describe 'Merge request > User sees versions', :js do end it 'show diff between new and old version' do - expect(page).to have_content '4 changed files' - expect(page).to have_content '15 additions 6 deletions' + additions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-addition') + .ancestor('.diff-stats-group').text + deletions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-deletion') + .ancestor('.diff-stats-group').text + + expect(page).to have_content '4 Files' + expect(additions_content).to eq '15' + expect(deletions_content).to eq '6' end it 'returns to latest version when "Show latest version" button is clicked' do @@ -150,7 +163,7 @@ describe 'Merge request > User sees versions', :js do page.within '.mr-version-dropdown' do expect(page).to have_content 'latest version' end - expect(page).to have_content '8 changed files' + expect(page).to have_content '8 Files' end it_behaves_like 'allows commenting', @@ -176,7 +189,7 @@ describe 'Merge request > User sees versions', :js do find('.btn-default').click click_link 'version 1' end - expect(page).to have_content '0 changed files' + expect(page).to have_content '0 Files' end end @@ -202,7 +215,7 @@ describe 'Merge request > User sees versions', :js do expect(page).to have_content 'version 1' end - expect(page).to have_content '0 changed files' + expect(page).to have_content '0 Files' end end diff --git a/spec/javascripts/diffs/components/compare_versions_spec.js b/spec/javascripts/diffs/components/compare_versions_spec.js index 2f0385454d7..e886f962d2f 100644 --- a/spec/javascripts/diffs/components/compare_versions_spec.js +++ b/spec/javascripts/diffs/components/compare_versions_spec.js @@ -10,6 +10,10 @@ describe('CompareVersions', () => { const targetBranch = { branchName: 'tmp-wine-dev', versionIndex: -1 }; beforeEach(() => { + store.state.diffs.addedLines = 10; + store.state.diffs.removedLines = 20; + store.state.diffs.diffFiles.push('test'); + vm = createComponentWithStore(Vue.extend(CompareVersionsComponent), store, { mergeRequestDiffs: diffsMockData, mergeRequestDiff: diffsMockData[0], diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index b77907ff26f..787a81fd88f 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -24,6 +24,10 @@ describe('diff_file_header', () => { beforeEach(() => { const diffFile = diffDiscussionMock.diff_file; + + diffFile.added_lines = 2; + diffFile.removed_lines = 1; + props = { diffFile: { ...diffFile }, canCurrentUserFork: false, diff --git a/spec/javascripts/diffs/components/diff_stats_spec.js b/spec/javascripts/diffs/components/diff_stats_spec.js new file mode 100644 index 00000000000..984b3026209 --- /dev/null +++ b/spec/javascripts/diffs/components/diff_stats_spec.js @@ -0,0 +1,33 @@ +import { shallowMount } from '@vue/test-utils'; +import DiffStats from '~/diffs/components/diff_stats.vue'; + +describe('diff_stats', () => { + it('does not render a group if diffFileLengths is not passed in', () => { + const wrapper = shallowMount(DiffStats, { + propsData: { + addedLines: 1, + removedLines: 2, + }, + }); + const groups = wrapper.findAll('.diff-stats-group'); + + expect(groups.length).toBe(2); + }); + + it('shows amount of files changed, lines added and lines removed when passed all props', () => { + const wrapper = shallowMount(DiffStats, { + propsData: { + addedLines: 100, + removedLines: 200, + diffFilesLength: 300, + }, + }); + const additions = wrapper.find('icon-stub[name="file-addition"]').element.parentNode; + const deletions = wrapper.find('icon-stub[name="file-deletion"]').element.parentNode; + const filesChanged = wrapper.find('icon-stub[name="doc-code"]').element.parentNode; + + expect(additions.textContent).toContain('100'); + expect(deletions.textContent).toContain('200'); + expect(filesChanged.textContent).toContain('300'); + }); +}); diff --git a/spec/javascripts/diffs/components/tree_list_spec.js b/spec/javascripts/diffs/components/tree_list_spec.js index 08b0b4f9e45..cf197bd00b0 100644 --- a/spec/javascripts/diffs/components/tree_list_spec.js +++ b/spec/javascripts/diffs/components/tree_list_spec.js @@ -35,12 +35,6 @@ describe('Diffs tree list component', () => { vm.$destroy(); }); - it('renders diff stats', () => { - expect(vm.$el.textContent).toContain('1 changed file'); - expect(vm.$el.textContent).toContain('10 additions'); - expect(vm.$el.textContent).toContain('20 deletions'); - }); - it('renders empty text', () => { expect(vm.$el.textContent).toContain('No files found'); }); |