Merge branch '239861-remove-negative-margin-top' into 'master'

Remove negative-margin-top class

See merge request gitlab-org/gitlab!42958

app/assets/javascripts/commit/pipelines/pipelines_table.vue

 <script>
 import { GlButton, GlLoadingIcon, GlModal, GlLink } from '@gitlab/ui';
-import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
 import PipelinesService from '~/pipelines/services/pipelines_service';
 import PipelineStore from '~/pipelines/stores/pipelines_store';
 import pipelinesMixin from '~/pipelines/mixins/pipelines';
@@ -126,16 +125,6 @@ export default {
         (latest.flags.detached_merge_request_pipeline || latest.flags.merge_request_pipeline)
       );
     },
-    /**
-     * When we are on Desktop and the button is visible
-     * we need to add a negative margin to the table
-     * to make it inline with the button
-     *
-     * @returns {Boolean}
-     */
-    shouldAddNegativeMargin() {
-      return this.canRenderPipelineButton && bp.isDesktop();
-    },
   },
   created() {
     this.service = new PipelinesService(this.endpoint);
@@ -205,65 +194,76 @@ export default {
     />
 
     <div v-else-if="shouldRenderTable" class="table-holder">
-      <div v-if="canRenderPipelineButton" class="nav justify-content-end">
-        <gl-button
-          variant="success"
-          class="js-run-mr-pipeline gl-mt-3 btn-wide-on-xs"
-          :disabled="state.isRunningMergeRequestPipeline"
-          @click="tryRunPipeline"
-        >
-          <gl-loading-icon v-if="state.isRunningMergeRequestPipeline" inline />
-          {{ s__('Pipelines|Run Pipeline') }}
-        </gl-button>
-
-        <gl-modal
-          :id="modalId"
-          ref="modal"
-          :modal-id="modalId"
-          :title="s__('Pipelines|Are you sure you want to run this pipeline?')"
-          :ok-title="s__('Pipelines|Run Pipeline')"
-          ok-variant="danger"
-          @ok="onClickRunPipeline"
-        >
-          <p>
-            {{
-              s__(
-                'Pipelines|This pipeline will run code originating from a forked project merge request. This means that the code can potentially have security considerations like exposing CI variables.',
-              )
-            }}
-          </p>
-          <p>
-            {{
-              s__(
-                "Pipelines|It is recommended the code is reviewed thoroughly before running this pipeline with the parent project's CI resource.",
-              )
-            }}
-          </p>
-          <p>
-            {{
-              s__(
-                'Pipelines|If you are unsure, please ask a project maintainer to review it for you.',
-              )
-            }}
-          </p>
-          <gl-link
-            href="/help/ci/merge_request_pipelines/index.html#run-pipelines-in-the-parent-project-for-merge-requests-from-a-forked-project"
-            target="_blank"
-          >
-            {{ s__('Pipelines|More Information') }}
-          </gl-link>
-        </gl-modal>
-      </div>
+      <gl-button
+        v-if="canRenderPipelineButton"
+        block
+        class="gl-mt-3 gl-mb-0 gl-display-md-none"
+        variant="success"
+        data-testid="run_pipeline_button_mobile"
+        :loading="state.isRunningMergeRequestPipeline"
+        @click="tryRunPipeline"
+      >
+        {{ s__('Pipelines|Run Pipeline') }}
+      </gl-button>
 
       <pipelines-table-component
         :pipelines="state.pipelines"
         :update-graph-dropdown="updateGraphDropdown"
         :auto-devops-help-path="autoDevopsHelpPath"
         :view-type="viewType"
-        :class="{ 'negative-margin-top': shouldAddNegativeMargin }"
-      />
+      >
+        <template #table-header-actions>
+          <div v-if="canRenderPipelineButton" class="gl-text-right">
+            <gl-button
+              variant="success"
+              data-testid="run_pipeline_button"
+              :loading="state.isRunningMergeRequestPipeline"
+              @click="tryRunPipeline"
+            >
+              {{ s__('Pipelines|Run Pipeline') }}
+            </gl-button>
+          </div>
+        </template>
+      </pipelines-table-component>
     </div>
 
+    <gl-modal
+      v-if="canRenderPipelineButton"
+      :id="modalId"
+      ref="modal"
+      :modal-id="modalId"
+      :title="s__('Pipelines|Are you sure you want to run this pipeline?')"
+      :ok-title="s__('Pipelines|Run Pipeline')"
+      ok-variant="danger"
+      @ok="onClickRunPipeline"
+    >
+      <p>
+        {{
+          s__(
+            'Pipelines|This pipeline will run code originating from a forked project merge request. This means that the code can potentially have security considerations like exposing CI variables.',
+          )
+        }}
+      </p>
+      <p>
+        {{
+          s__(
+            "Pipelines|It is recommended the code is reviewed thoroughly before running this pipeline with the parent project's CI resource.",
+          )
+        }}
+      </p>
+      <p>
+        {{
+          s__('Pipelines|If you are unsure, please ask a project maintainer to review it for you.')
+        }}
+      </p>
+      <gl-link
+        href="/help/ci/merge_request_pipelines/index.html#run-pipelines-in-the-parent-project-for-merge-requests-from-a-forked-project"
+        target="_blank"
+      >
+        {{ s__('Pipelines|More Information') }}
+      </gl-link>
+    </gl-modal>
+
     <table-pagination
       v-if="shouldRenderPagination"
       :change="onChangePage"

app/assets/javascripts/pipelines/components/pipelines_list/pipelines_table.vue

@@ -91,6 +91,10 @@ export default {
       <div class="table-section section-15 js-pipeline-stages pipeline-stages" role="rowheader">
         {{ s__('Pipeline|Stages') }}
       </div>
+      <div class="table-section section-15" role="rowheader"></div>
+      <div class="table-section section-20" role="rowheader">
+        <slot name="table-header-actions"></slot>
+      </div>
     </div>
     <pipelines-table-row-component
       v-for="model in pipelines"

app/assets/stylesheets/framework/buttons.scss

@@ -417,12 +417,6 @@
   }
 }
 
-@include media-breakpoint-down(xs) {
-  .btn-wide-on-xs {
-    width: 100%;
-  }
-}
-
 .btn-blank {
   padding: 0;
   background: transparent;

app/assets/stylesheets/framework/variables.scss

@@ -819,7 +819,6 @@ $pipeline-dropdown-line-height: 20px;
 $pipeline-dropdown-status-icon-size: 18px;
 $ci-action-dropdown-button-size: 24px;
 $ci-action-dropdown-svg-size: 12px;
-$pipelines-table-header-height: 40px;
 
 /*
 CI variable lists

app/assets/stylesheets/pages/pipelines.scss

@@ -26,10 +26,6 @@
 }
 
 .pipelines {
-  .negative-margin-top {
-    margin-top: -$pipelines-table-header-height;
-  }
-
   .stage {
     max-width: 90px;
     width: 90px;

spec/features/merge_request/user_sees_pipelines_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
 
           wait_for_requests
 
-          expect(page.find('.js-run-mr-pipeline')).to have_text('Run Pipeline')
+          expect(page.find('[data-testid="run_pipeline_button"]')).to have_text('Run Pipeline')
         end
       end
 
@@ -66,7 +66,7 @@ RSpec.describe 'Merge request > User sees pipelines', :js do
 
           wait_for_requests
 
-          expect(page.find('.js-run-mr-pipeline')).to have_text('Run Pipeline')
+          expect(page.find('[data-testid="run_pipeline_button"]')).to have_text('Run Pipeline')
         end
       end
     end

spec/frontend/commit/pipelines/pipelines_spec.js

@@ -21,6 +21,10 @@ describe('Pipelines table in Commits and Merge requests', () => {
 
   preloadFixtures(jsonFixtureName);
 
+  const findRunPipelineBtn = () => vm.$el.querySelector('[data-testid="run_pipeline_button"]');
+  const findRunPipelineBtnMobile = () =>
+    vm.$el.querySelector('[data-testid="run_pipeline_button_mobile"]');
+
   beforeEach(() => {
     mock = new MockAdapter(axios);
 
@@ -131,7 +135,8 @@ describe('Pipelines table in Commits and Merge requests', () => {
         vm = mountComponent(PipelinesTable, { ...props });
 
         setImmediate(() => {
-          expect(vm.$el.querySelector('.js-run-mr-pipeline')).not.toBeNull();
+          expect(findRunPipelineBtn()).not.toBeNull();
+          expect(findRunPipelineBtnMobile()).not.toBeNull();
           done();
         });
       });
@@ -147,7 +152,8 @@ describe('Pipelines table in Commits and Merge requests', () => {
         vm = mountComponent(PipelinesTable, { ...props });
 
         setImmediate(() => {
-          expect(vm.$el.querySelector('.js-run-mr-pipeline')).toBeNull();
+          expect(findRunPipelineBtn()).toBeNull();
+          expect(findRunPipelineBtnMobile()).toBeNull();
           done();
         });
       });
@@ -157,7 +163,7 @@ describe('Pipelines table in Commits and Merge requests', () => {
       const findModal = () =>
         document.querySelector('#create-pipeline-for-fork-merge-request-modal');
 
-      beforeEach(() => {
+      beforeEach(done => {
         pipelineCopy.flags.detached_merge_request_pipeline = true;
 
         mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
@@ -168,23 +174,46 @@ describe('Pipelines table in Commits and Merge requests', () => {
           projectId: '5',
           mergeRequestId: 3,
         });
-      });
 
-      it('updates the loading state', done => {
         jest.spyOn(Api, 'postMergeRequestPipeline').mockReturnValue(Promise.resolve());
 
         setImmediate(() => {
-          vm.$el.querySelector('.js-run-mr-pipeline').click();
+          done();
+        });
+      });
 
-          vm.$nextTick(() => {
-            expect(findModal()).toBeNull();
-            expect(vm.state.isRunningMergeRequestPipeline).toBe(true);
+      it('on desktop, shows a loading button', done => {
+        findRunPipelineBtn().click();
 
-            setImmediate(() => {
-              expect(vm.state.isRunningMergeRequestPipeline).toBe(false);
+        vm.$nextTick(() => {
+          expect(findModal()).toBeNull();
 
-              done();
-            });
+          expect(findRunPipelineBtn().disabled).toBe(true);
+          expect(findRunPipelineBtn().querySelector('.gl-spinner')).not.toBeNull();
+
+          setImmediate(() => {
+            expect(findRunPipelineBtn().disabled).toBe(false);
+            expect(findRunPipelineBtn().querySelector('.gl-spinner')).toBeNull();
+
+            done();
+          });
+        });
+      });
+
+      it('on mobile, shows a loading button', done => {
+        findRunPipelineBtnMobile().click();
+
+        vm.$nextTick(() => {
+          expect(findModal()).toBeNull();
+
+          expect(findModal()).toBeNull();
+          expect(findRunPipelineBtn().querySelector('.gl-spinner')).not.toBeNull();
+
+          setImmediate(() => {
+            expect(findRunPipelineBtn().disabled).toBe(false);
+            expect(findRunPipelineBtn().querySelector('.gl-spinner')).toBeNull();
+
+            done();
           });
         });
       });
@@ -194,7 +223,7 @@ describe('Pipelines table in Commits and Merge requests', () => {
       const findModal = () =>
         document.querySelector('#create-pipeline-for-fork-merge-request-modal');
 
-      beforeEach(() => {
+      beforeEach(done => {
         pipelineCopy.flags.detached_merge_request_pipeline = true;
 
         mock.onGet('endpoint.json').reply(200, [pipelineCopy]);
@@ -207,18 +236,29 @@ describe('Pipelines table in Commits and Merge requests', () => {
           sourceProjectFullPath: 'test/parent-project',
           targetProjectFullPath: 'test/fork-project',
         });
-      });
 
-      it('shows a security warning modal', done => {
         jest.spyOn(Api, 'postMergeRequestPipeline').mockReturnValue(Promise.resolve());
 
         setImmediate(() => {
-          vm.$el.querySelector('.js-run-mr-pipeline').click();
+          done();
+        });
+      });
 
-          vm.$nextTick(() => {
-            expect(findModal()).not.toBeNull();
-            done();
-          });
+      it('on desktop, shows a security warning modal', done => {
+        findRunPipelineBtn().click();
+
+        vm.$nextTick(() => {
+          expect(findModal()).not.toBeNull();
+          done();
+        });
+      });
+
+      it('on mobile, shows a security warning modal', done => {
+        findRunPipelineBtnMobile().click();
+
+        vm.$nextTick(() => {
+          expect(findModal()).not.toBeNull();
+          done();
         });
       });
     });