diff options
author | Sam Bigelow <sbigelow@gitlab.com> | 2019-03-01 16:41:16 -0500 |
---|---|---|
committer | Sam Bigelow <sbigelow@gitlab.com> | 2019-03-21 10:24:18 -0400 |
commit | 1a14e5230e5b7f705fc09c3baf46f6cf74ca3ad0 (patch) | |
tree | 7c8f1d88347252e88535dc087f3129102ef77bf8 | |
parent | c174fc0cc1a221bb2ab8f265b3691a17d5cfd8aa (diff) | |
download | gitlab-ce-1a14e5230e5b7f705fc09c3baf46f6cf74ca3ad0.tar.gz |
Add merge request popover with details
- Show pipeline status, title, MR Status and project path
- Popover attached to gitlab flavored markdown everywhere, including:
+ MR/Issue Title
+ MR/Issue description
+ MR/Issue comments
+ Rendered markdown files
18 files changed, 438 insertions, 13 deletions
diff --git a/app/assets/javascripts/behaviors/markdown/render_gfm.js b/app/assets/javascripts/behaviors/markdown/render_gfm.js index fc9286d15e6..bfb073fdcdc 100644 --- a/app/assets/javascripts/behaviors/markdown/render_gfm.js +++ b/app/assets/javascripts/behaviors/markdown/render_gfm.js @@ -4,6 +4,7 @@ import renderMath from './render_math'; import renderMermaid from './render_mermaid'; import highlightCurrentUser from './highlight_current_user'; import initUserPopovers from '../../user_popovers'; +import initMRPopovers from '../../mr_popover'; // Render GitLab flavoured Markdown // @@ -15,6 +16,7 @@ $.fn.renderGFM = function renderGFM() { renderMermaid(this.find('.js-render-mermaid')); highlightCurrentUser(this.find('.gfm-project_member').get()); initUserPopovers(this.find('.gfm-project_member').get()); + initMRPopovers(this.find('.gfm-merge_request').get()); return this; }; diff --git a/app/assets/javascripts/mr_popover/components/mr_popover.vue b/app/assets/javascripts/mr_popover/components/mr_popover.vue new file mode 100644 index 00000000000..8e2d8fa816a --- /dev/null +++ b/app/assets/javascripts/mr_popover/components/mr_popover.vue @@ -0,0 +1,110 @@ +<script> +import { GlPopover, GlSkeletonLoading } from '@gitlab/ui'; +import Icon from '../../vue_shared/components/icon.vue'; +import CiIcon from '../../vue_shared/components/ci_icon.vue'; +import timeagoMixin from '../../vue_shared/mixins/timeago'; +import query from '../queries/merge_request.graphql'; +import { mrStates, humanMRStates } from '../constants'; + +export default { + name: 'MRPopover', + components: { + GlPopover, + GlSkeletonLoading, + Icon, + CiIcon, + }, + mixins: [timeagoMixin], + props: { + target: { + type: HTMLAnchorElement, + required: true, + }, + projectPath: { + type: String, + required: true, + }, + mergeRequestIID: { + type: String, + required: true, + }, + mergeRequestTitle: { + type: String, + required: true, + }, + }, + data() { + return { + mergeRequest: {}, + }; + }, + computed: { + detailedStatus() { + return this.mergeRequest.headPipeline && this.mergeRequest.headPipeline.detailedStatus; + }, + formattedTime() { + return this.timeFormated(this.mergeRequest.createdAt); + }, + statusBoxClass() { + switch (this.mergeRequest.state) { + case mrStates.merged: + return 'status-box-mr-merged'; + case mrStates.closed: + return 'status-box-closed'; + default: + return 'status-box-open'; + } + }, + stateHumanName() { + switch (this.mergeRequest.state) { + case mrStates.merged: + return humanMRStates.merged; + case mrStates.closed: + return humanMRStates.closed; + default: + return humanMRStates.open; + } + }, + showDetails() { + return Object.keys(this.mergeRequest).length > 0; + }, + }, + apollo: { + mergeRequest: { + query, + update: data => data.project.mergeRequest, + variables() { + const { projectPath, mergeRequestIID } = this; + + return { + projectPath, + mergeRequestIID, + }; + }, + }, + }, +}; +</script> + +<template> + <gl-popover :target="target" boundary="viewport" placement="top" show> + <div class="mr-popover"> + <div v-if="$apollo.loading"> + <gl-skeleton-loading :lines="1" class="animation-container-small mt-1" /> + </div> + <div v-else-if="showDetails" class="d-flex align-items-center justify-content-between"> + <div class="d-inline-flex align-items-center"> + <div :class="`issuable-status-box status-box ${statusBoxClass}`"> + {{ stateHumanName }} + </div> + <span class="text-secondary">Opened <time v-text="formattedTime"></time></span> + </div> + <ci-icon v-if="detailedStatus" :status="detailedStatus" /> + </div> + <h5 class="my-2">{{ mergeRequestTitle }}</h5> + <div class="text-secondary"> + {{ `${projectPath}!${mergeRequestIID}` }} + </div> + </div> + </gl-popover> +</template> diff --git a/app/assets/javascripts/mr_popover/constants.js b/app/assets/javascripts/mr_popover/constants.js new file mode 100644 index 00000000000..433df844c80 --- /dev/null +++ b/app/assets/javascripts/mr_popover/constants.js @@ -0,0 +1,10 @@ +export const mrStates = { + merged: 'merged', + closed: 'closed', +}; + +export const humanMRStates = { + merged: 'Merged', + closed: 'Closed', + open: 'Open', +}; diff --git a/app/assets/javascripts/mr_popover/index.js b/app/assets/javascripts/mr_popover/index.js new file mode 100644 index 00000000000..cc686b401d2 --- /dev/null +++ b/app/assets/javascripts/mr_popover/index.js @@ -0,0 +1,62 @@ +import Vue from 'vue'; +import VueApollo from 'vue-apollo'; +import MRPopover from './components/mr_popover.vue'; +import createDefaultClient from '~/lib/graphql'; + +let renderedPopover; +let renderFn; + +const handleUserPopoverMouseOut = ({ target }) => { + target.removeEventListener('mouseleave', handleUserPopoverMouseOut); + + if (renderFn) { + clearTimeout(renderFn); + } + if (renderedPopover) { + renderedPopover.$destroy(); + renderedPopover = null; + } +}; + +/** + * Adds a MergeRequestPopover component to the body, hands over as much data as the target element has in data attributes. + * loads based on data-project-path and data-iid more data about an MR from the API and sets it on the popover + */ +const handleMRPopoverMount = apolloProvider => ({ target }) => { + // Add listener to actually remove it again + target.addEventListener('mouseleave', handleUserPopoverMouseOut); + + const { projectPath, mrTitle, iid } = target.dataset; + const mergeRequest = {}; + + renderFn = setTimeout(() => { + const MRPopoverComponent = Vue.extend(MRPopover); + renderedPopover = new MRPopoverComponent({ + propsData: { + target, + projectPath, + mergeRequestIID: iid, + mergeRequest, + mergeRequestTitle: mrTitle, + }, + apolloProvider, + }); + + renderedPopover.$mount(); + }, 200); // 200ms delay so not every mouseover triggers Popover + API Call +}; + +export default elements => { + const mrLinks = elements || [...document.querySelectorAll('.gfm-merge_request')]; + if (mrLinks.length > 0) { + Vue.use(VueApollo); + + const apolloProvider = new VueApollo({ + defaultClient: createDefaultClient(), + }); + + mrLinks.forEach(el => { + el.addEventListener('mouseenter', handleMRPopoverMount(apolloProvider)); + }); + } +}; diff --git a/app/assets/javascripts/mr_popover/queries/merge_request.graphql b/app/assets/javascripts/mr_popover/queries/merge_request.graphql new file mode 100644 index 00000000000..0bb9bc03bc7 --- /dev/null +++ b/app/assets/javascripts/mr_popover/queries/merge_request.graphql @@ -0,0 +1,14 @@ +query mergeRequest($projectPath: ID!, $mergeRequestIID: ID!) { + project(fullPath: $projectPath) { + mergeRequest(iid: $mergeRequestIID) { + createdAt + state + headPipeline { + detailedStatus { + icon + group + } + } + } + } +} diff --git a/app/assets/javascripts/vue_shared/components/ci_icon.vue b/app/assets/javascripts/vue_shared/components/ci_icon.vue index e6f0a1c69cd..25f80219993 100644 --- a/app/assets/javascripts/vue_shared/components/ci_icon.vue +++ b/app/assets/javascripts/vue_shared/components/ci_icon.vue @@ -22,6 +22,7 @@ import Icon from '../../vue_shared/components/icon.vue'; * - Jobs show view header * - Jobs show view sidebar * - Linked pipelines + * - Extended MR Popover */ const validSizes = [8, 12, 16, 18, 24, 32, 48, 72]; diff --git a/app/assets/stylesheets/components/popover.scss b/app/assets/stylesheets/components/popover.scss index 2f4d30fe923..7d46b262a69 100644 --- a/app/assets/stylesheets/components/popover.scss +++ b/app/assets/stylesheets/components/popover.scss @@ -7,3 +7,10 @@ line-height: $gl-line-height; } } + +.mr-popover { + .text-secondary { + font-size: 12px; + line-height: 1.33; + } +} diff --git a/app/graphql/types/ci/detailed_status_type.rb b/app/graphql/types/ci/detailed_status_type.rb new file mode 100644 index 00000000000..2987354b556 --- /dev/null +++ b/app/graphql/types/ci/detailed_status_type.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true +module Types + module Ci + class DetailedStatusType < BaseObject + graphql_name 'DetailedStatus' + + field :group, GraphQL::STRING_TYPE, null: false + field :icon, GraphQL::STRING_TYPE, null: false + field :favicon, GraphQL::STRING_TYPE, null: false + field :details_path, GraphQL::STRING_TYPE, null: false + field :has_details, GraphQL::BOOLEAN_TYPE, null: false, method: :has_details? + field :label, GraphQL::STRING_TYPE, null: false + field :text, GraphQL::STRING_TYPE, null: false + field :tooltip, GraphQL::STRING_TYPE, null: false, method: :status_tooltip + end + end +end diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb index 2bbffad4563..18696293b97 100644 --- a/app/graphql/types/ci/pipeline_type.rb +++ b/app/graphql/types/ci/pipeline_type.rb @@ -13,6 +13,10 @@ module Types field :sha, GraphQL::STRING_TYPE, null: false field :before_sha, GraphQL::STRING_TYPE, null: true field :status, PipelineStatusEnum, null: false + field :detailed_status, + Types::Ci::DetailedStatusType, + null: false, + resolve: -> (obj, _args, ctx) { obj.detailed_status(ctx[:current_user]) } field :duration, GraphQL::INT_TYPE, null: true, diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 1a8570b80c3..9aa947eceda 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -14,7 +14,7 @@ module CacheMarkdownField # Increment this number every time the renderer changes its output CACHE_COMMONMARK_VERSION_START = 10 - CACHE_COMMONMARK_VERSION = 14 + CACHE_COMMONMARK_VERSION = 15 # changes to these attributes cause the cache to be invalidates INVALIDATED_BY = %w[author project].freeze diff --git a/changelogs/unreleased/54916-extended-tooltip-for-merge-request-links.yml b/changelogs/unreleased/54916-extended-tooltip-for-merge-request-links.yml new file mode 100644 index 00000000000..7fd0bcd1c00 --- /dev/null +++ b/changelogs/unreleased/54916-extended-tooltip-for-merge-request-links.yml @@ -0,0 +1,5 @@ +--- +title: Add extended merge request tooltip +merge_request: !25221 +author: +type: added diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index 4764f8e1e19..5f8aca104aa 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -181,9 +181,10 @@ module Banzai title = object_link_title(object, matches) klass = reference_class(object_sym) - data = data_attributes_for(link_content || match, parent, object, - link_content: !!link_content, - link_reference: link_reference) + data_attributes = data_attributes_for(link_content || match, parent, object, + link_content: !!link_content, + link_reference: link_reference) + data = data_attribute(data_attributes) url = if matches.names.include?("url") && matches[:url] @@ -206,13 +207,13 @@ module Banzai def data_attributes_for(text, parent, object, link_content: false, link_reference: false) object_parent_type = parent.is_a?(Group) ? :group : :project - data_attribute( + { original: text, link: link_content, link_reference: link_reference, object_parent_type => parent.id, object_sym => object.id - ) + } end def object_link_text_extras(object, matches) diff --git a/lib/banzai/filter/merge_request_reference_filter.rb b/lib/banzai/filter/merge_request_reference_filter.rb index 7098767b583..f05902078dc 100644 --- a/lib/banzai/filter/merge_request_reference_filter.rb +++ b/lib/banzai/filter/merge_request_reference_filter.rb @@ -20,7 +20,9 @@ module Banzai end def object_link_title(object, matches) - object_link_commit_title(object, matches) || super + # The method will return `nil` if object is not a commit + # allowing for properly handling the extended MR Tooltip + object_link_commit_title(object, matches) end def object_link_text_extras(object, matches) @@ -53,6 +55,14 @@ module Banzai .includes(target_project: :namespace) end + def reference_class(object_sym, options = {}) + super(object_sym, tooltip: false) + end + + def data_attributes_for(text, parent, object, data = {}) + super.merge(project_path: parent.full_path, iid: object.iid, mr_title: object.title) + end + private def object_link_commit_title(object, matches) diff --git a/spec/frontend/mr_popover/__snapshots__/mr_popover_spec.js.snap b/spec/frontend/mr_popover/__snapshots__/mr_popover_spec.js.snap new file mode 100644 index 00000000000..5f9f13d591d --- /dev/null +++ b/spec/frontend/mr_popover/__snapshots__/mr_popover_spec.js.snap @@ -0,0 +1,93 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MR Popover loaded state matches the snapshot 1`] = ` +<glpopover-stub + boundary="viewport" + placement="top" + show="" + target="" +> + <div + class="mr-popover" + > + <div + class="d-flex align-items-center justify-content-between" + > + <div + class="d-inline-flex align-items-center" + > + <div + class="issuable-status-box status-box status-box-open" + > + + Open + + </div> + + <span + class="text-secondary" + > + Opened + <time> + just now + </time> + </span> + </div> + + <ciicon-stub + cssclasses="" + size="16" + status="[object Object]" + /> + </div> + + <h5 + class="my-2" + > + MR Title + </h5> + + <div + class="text-secondary" + > + + foo/bar!1 + + </div> + </div> +</glpopover-stub> +`; + +exports[`MR Popover shows skeleton-loader while apollo is loading 1`] = ` +<glpopover-stub + boundary="viewport" + placement="top" + show="" + target="" +> + <div + class="mr-popover" + > + <div> + <glskeletonloading-stub + class="animation-container-small mt-1" + lines="1" + /> + </div> + + <h5 + class="my-2" + > + MR Title + </h5> + + <div + class="text-secondary" + > + + foo/bar!1 + + </div> + </div> +</glpopover-stub> +`; diff --git a/spec/frontend/mr_popover/mr_popover_spec.js b/spec/frontend/mr_popover/mr_popover_spec.js new file mode 100644 index 00000000000..79ed4163010 --- /dev/null +++ b/spec/frontend/mr_popover/mr_popover_spec.js @@ -0,0 +1,61 @@ +import MRPopover from '~/mr_popover/components/mr_popover'; +import { shallowMount } from '@vue/test-utils'; + +describe('MR Popover', () => { + let wrapper; + + beforeEach(() => { + wrapper = shallowMount(MRPopover, { + propsData: { + target: document.createElement('a'), + projectPath: 'foo/bar', + mergeRequestIID: '1', + mergeRequestTitle: 'MR Title', + }, + mocks: { + $apollo: { + loading: false, + }, + }, + }); + }); + + it('shows skeleton-loader while apollo is loading', () => { + wrapper.vm.$apollo.loading = true; + + expect(wrapper.element).toMatchSnapshot(); + }); + + describe('loaded state', () => { + it('matches the snapshot', () => { + wrapper.setData({ + mergeRequest: { + state: 'opened', + createdAt: new Date(), + headPipeline: { + detailedStatus: { + group: 'success', + status: 'status_success', + }, + }, + }, + }); + + expect(wrapper.element).toMatchSnapshot(); + }); + + it('does not show CI Icon if there is no pipeline data', () => { + wrapper.setData({ + mergeRequest: { + state: 'opened', + headPipeline: null, + stateHumanName: 'Open', + title: 'Merge Request Title', + createdAt: new Date(), + }, + }); + + expect(wrapper.contains('ciicon-stub')).toBe(false); + }); + }); +}); diff --git a/spec/graphql/types/ci/detailed_status_type_spec.rb b/spec/graphql/types/ci/detailed_status_type_spec.rb new file mode 100644 index 00000000000..a21162adb42 --- /dev/null +++ b/spec/graphql/types/ci/detailed_status_type_spec.rb @@ -0,0 +1,11 @@ +require 'spec_helper' + +describe Types::Ci::DetailedStatusType do + it { expect(described_class.graphql_name).to eq('DetailedStatus') } + + it "has all fields" do + expect(described_class).to have_graphql_fields(:group, :icon, :favicon, + :details_path, :has_details, + :label, :text, :tooltip) + end +end diff --git a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb index 55c41e55437..72dfd6ff9ea 100644 --- a/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/merge_request_reference_filter_spec.rb @@ -30,6 +30,23 @@ describe Banzai::Filter::MergeRequestReferenceFilter do end end + describe 'all references' do + let(:doc) { reference_filter(merge.to_reference) } + let(:tag_el) { doc.css('a').first } + + it 'adds merge request iid' do + expect(tag_el["data-iid"]).to eq(merge.iid.to_s) + end + + it 'adds project data attribute with project id' do + expect(tag_el["data-project-path"]).to eq(project.full_path) + end + + it 'does not add `has-tooltip` class' do + expect(tag_el["class"]).not_to include('has-tooltip') + end + end + context 'internal reference' do let(:reference) { merge.to_reference } @@ -57,9 +74,9 @@ describe Banzai::Filter::MergeRequestReferenceFilter do expect(reference_filter(act).to_html).to eq exp end - it 'includes a title attribute' do + it 'has no title' do doc = reference_filter("Merge #{reference}") - expect(doc.css('a').first.attr('title')).to eq merge.title + expect(doc.css('a').first.attr('title')).to eq "" end it 'escapes the title attribute' do @@ -69,9 +86,9 @@ describe Banzai::Filter::MergeRequestReferenceFilter do expect(doc.text).to eq "Merge #{reference}" end - it 'includes default classes' do + it 'includes default classes, without tooltip' do doc = reference_filter("Merge #{reference}") - expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-merge_request has-tooltip' + expect(doc.css('a').first.attr('class')).to eq 'gfm gfm-merge_request' end it 'includes a data-project attribute' do diff --git a/spec/requests/api/graphql/project/merge_request_spec.rb b/spec/requests/api/graphql/project/merge_request_spec.rb index deb6abbc026..74820d39102 100644 --- a/spec/requests/api/graphql/project/merge_request_spec.rb +++ b/spec/requests/api/graphql/project/merge_request_spec.rb @@ -70,13 +70,13 @@ describe 'getting merge request information nested in a project' do context 'when there are pipelines' do before do - pipeline = create( + create( :ci_pipeline, project: merge_request.source_project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha ) - merge_request.update!(head_pipeline: pipeline) + merge_request.update_head_pipeline end it 'has a head pipeline' do |