Enhance UX on Environment Form with Loading Icon

The loading icon adds nice feedback to the user that _something_ is
happening, but we only unset it in the error case as the good case (in
both editing and creating) navigate us to a new page anyway.

We also add some clientside validations following the pajamas guideline.

Finally, delete the HAML form. We don't need it anymore.

Changelog: changed

app/assets/javascripts/environments/components/edit_environment.vue

@@ -21,6 +21,7 @@ export default {
         name: this.environment.name,
         externalUrl: this.environment.external_url,
       },
+      loading: false,
     };
   },
   methods: {
@@ -28,6 +29,7 @@ export default {
       this.formEnvironment = environment;
     },
     onSubmit() {
+      this.loading = true;
       axios
         .put(this.updateEnvironmentPath, {
           id: this.environment.id,
@@ -38,6 +40,7 @@ export default {
         .catch((error) => {
           const message = error.response.data.message[0];
           createFlash({ message });
+          this.loading = false;
         });
     },
   },
@@ -48,6 +51,7 @@ export default {
     :cancel-path="projectEnvironmentsPath"
     :environment="formEnvironment"
     :title="__('Edit environment')"
+    :loading="loading"
     @change="onChange"
     @submit="onSubmit"
   />

app/assets/javascripts/environments/components/environment_form.vue

@@ -26,6 +26,11 @@ export default {
       required: true,
       type: String,
     },
+    loading: {
+      required: false,
+      type: Boolean,
+      default: false,
+    },
   },
   i18n: {
     header: __('Environments'),
@@ -42,21 +47,26 @@ export default {
   helpPagePath: helpPagePath('ci/environments/index.md'),
   data() {
     return {
-      errors: {
+      visited: {
         name: null,
         url: null,
       },
     };
   },
+  computed: {
+    valid() {
+      return {
+        name: this.visited.name && this.environment.name !== '',
+        url: this.visited.url && isAbsolute(this.environment.externalUrl),
+      };
+    },
+  },
   methods: {
     onChange(env) {
       this.$emit('change', env);
     },
-    validateUrl() {
-      this.errors.url = isAbsolute(this.environment.externalUrl);
-    },
-    validateName() {
-      this.errors.name = this.environment.name !== '';
+    visit(field) {
+      this.visited[field] = true;
     },
   },
 };
@@ -89,40 +99,45 @@ export default {
         <gl-form-group
           :label="$options.i18n.nameLabel"
           label-for="environment_name"
-          :state="errors.name"
+          :state="valid.name"
           :invalid-feedback="$options.i18n.nameFeedback"
         >
           <gl-form-input
             id="environment_name"
             :value="environment.name"
-            :state="errors.name"
+            :state="valid.name"
             name="environment[name]"
             required
             @input="onChange({ ...environment, name: $event })"
-            @blur="validateName"
+            @blur="visit('name')"
           />
         </gl-form-group>
         <gl-form-group
           :label="$options.i18n.urlLabel"
-          :state="errors.url"
+          :state="valid.url"
           :invalid-feedback="$options.i18n.urlFeedback"
           label-for="environment_external_url"
         >
           <gl-form-input
             id="environment_external_url"
             :value="environment.externalUrl"
-            :state="errors.url"
+            :state="valid.url"
             name="environment[external_url]"
             type="url"
             @input="onChange({ ...environment, externalUrl: $event })"
-            @blur="validateUrl"
+            @blur="visit('url')"
           />
         </gl-form-group>
 
         <div class="form-actions">
-          <gl-button type="submit" variant="confirm" name="commit" class="js-no-auto-disable">{{
-            $options.i18n.save
-          }}</gl-button>
+          <gl-button
+            :loading="loading"
+            type="submit"
+            variant="confirm"
+            name="commit"
+            class="js-no-auto-disable"
+            >{{ $options.i18n.save }}</gl-button
+          >
           <gl-button :href="cancelPath">{{ $options.i18n.cancel }}</gl-button>
         </div>
       </gl-form>

app/assets/javascripts/environments/components/new_environment.vue

@@ -15,6 +15,7 @@ export default {
         name: '',
         externalUrl: '',
       },
+      loading: false,
     };
   },
   methods: {
@@ -22,6 +23,7 @@ export default {
       this.environment = env;
     },
     onSubmit() {
+      this.loading = true;
       axios
         .post(this.projectEnvironmentsPath, {
           name: this.environment.name,
@@ -31,6 +33,7 @@ export default {
         .catch((error) => {
           const message = error.response.data.message[0];
           createFlash({ message });
+          this.loading = false;
         });
     },
   },
@@ -41,6 +44,7 @@ export default {
     :cancel-path="projectEnvironmentsPath"
     :environment="environment"
     :title="__('New environment')"
+    :loading="loading"
     @change="onChange($event)"
     @submit="onSubmit"
   />

app/views/projects/environments/_form.html.haml

-.row.gl-mt-3.gl-mb-3
-  .col-lg-3
-    %h4.gl-mt-0
-      = _("Environments")
-    %p
-      - link_to_read_more = link_to(_("More information"), help_page_path("ci/environments/index.md"))
-      = _("Environments allow you to track deployments of your application %{link_to_read_more}.").html_safe % { link_to_read_more: link_to_read_more }
-
-  = form_for [@project, @environment], html: { class: 'col-lg-9' } do |f|
-    = form_errors(@environment)
-
-    .form-group
-      = f.label :name, _('Name'), class: 'label-bold'
-      = f.text_field :name, required: true, class: 'form-control'
-    .form-group
-      = f.label :external_url, _('External URL'), class: 'label-bold'
-      = f.url_field :external_url, class: 'form-control'
-
-    .form-actions
-      = f.submit _('Save'), class: 'gl-button btn btn-confirm'
-      = link_to _('Cancel'), project_environments_path(@project), class: 'gl-button btn btn-cancel'

locale/gitlab.pot

@@ -12470,9 +12470,6 @@ msgstr ""
 msgid "Environments Dashboard"
 msgstr ""
 
-msgid "Environments allow you to track deployments of your application %{link_to_read_more}."
-msgstr ""
-
 msgid "Environments allow you to track deployments of your application. %{linkStart}More information%{linkEnd}."
 msgstr ""
 

spec/frontend/environments/edit_environment_spec.js

+import { GlLoadingIcon } from '@gitlab/ui';
 import MockAdapter from 'axios-mock-adapter';
 import { mountExtended } from 'helpers/vue_test_utils_helper';
 import waitForPromises from 'helpers/wait_for_promises';
@@ -43,7 +44,9 @@ describe('~/environments/components/edit.vue', () => {
     wrapper.destroy();
   });
 
-  const fillForm = async (expected, response) => {
+  const showsLoading = () => wrapper.find(GlLoadingIcon).exists();
+
+  const submitForm = async (expected, response) => {
     mock
       .onPut(DEFAULT_OPTS.provide.updateEnvironmentPath, {
         name: expected.name,
@@ -72,10 +75,20 @@ describe('~/environments/components/edit.vue', () => {
     expect(input().element.value).toBe(value);
   });
 
+  it('shows loader after form is submitted', async () => {
+    const expected = { name: 'test', url: 'https://google.ca' };
+
+    expect(showsLoading()).toBe(false);
+
+    await submitForm(expected, [200, { path: '/test' }]);
+
+    expect(showsLoading()).toBe(true);
+  });
+
   it('submits the updated environment on submit', async () => {
     const expected = { name: 'test', url: 'https://google.ca' };
 
-    await fillForm(expected, [200, { path: '/test' }]);
+    await submitForm(expected, [200, { path: '/test' }]);
 
     expect(visitUrl).toHaveBeenCalledWith('/test');
   });
@@ -83,8 +96,9 @@ describe('~/environments/components/edit.vue', () => {
   it('shows errors on error', async () => {
     const expected = { name: 'test', url: 'https://google.ca' };
 
-    await fillForm(expected, [400, { message: ['name taken'] }]);
+    await submitForm(expected, [400, { message: ['name taken'] }]);
 
     expect(createFlash).toHaveBeenCalledWith({ message: 'name taken' });
+    expect(showsLoading()).toBe(false);
   });
 });

spec/frontend/environments/environment_form_spec.js

+import { GlLoadingIcon } from '@gitlab/ui';
 import { mountExtended } from 'helpers/vue_test_utils_helper';
 import EnvironmentForm from '~/environments/components/environment_form.vue';
 
 jest.mock('~/lib/utils/csrf');
 
-const DEFAULT_OPTS = {
-  propsData: {
-    environment: { name: '', externalUrl: '' },
-    title: 'environment',
-    cancelPath: '/cancel',
-  },
+const DEFAULT_PROPS = {
+  environment: { name: '', externalUrl: '' },
+  title: 'environment',
+  cancelPath: '/cancel',
 };
 
 describe('~/environments/components/form.vue', () => {
   let wrapper;
 
-  const createWrapper = (opts = {}) =>
+  const createWrapper = (propsData = {}) =>
     mountExtended(EnvironmentForm, {
-      ...DEFAULT_OPTS,
-      ...opts,
+      propsData: {
+        ...DEFAULT_PROPS,
+        ...propsData,
+      },
     });
 
-  beforeEach(() => {
-    wrapper = createWrapper();
-  });
-
   afterEach(() => {
     wrapper.destroy();
   });
 
-  it('links to documentation regarding environments', () => {
-    const link = wrapper.findByRole('link', { name: 'More information' });
-    expect(link.attributes('href')).toBe('/help/ci/environments/index.md');
-  });
-
-  it('links the cancel button to the cancel path', () => {
-    const cancel = wrapper.findByRole('link', { name: 'Cancel' });
+  describe('default', () => {
+    beforeEach(() => {
+      wrapper = createWrapper();
+    });
 
-    expect(cancel.attributes('href')).toBe(DEFAULT_OPTS.propsData.cancelPath);
-  });
+    it('links to documentation regarding environments', () => {
+      const link = wrapper.findByRole('link', { name: 'More information' });
+      expect(link.attributes('href')).toBe('/help/ci/environments/index.md');
+    });
 
-  describe('name input', () => {
-    let name;
+    it('links the cancel button to the cancel path', () => {
+      const cancel = wrapper.findByRole('link', { name: 'Cancel' });
 
-    beforeEach(() => {
-      name = wrapper.findByLabelText('Name');
+      expect(cancel.attributes('href')).toBe(DEFAULT_PROPS.cancelPath);
     });
 
-    it('should emit changes to the name', async () => {
-      await name.setValue('test');
-      await name.trigger('blur');
+    describe('name input', () => {
+      let name;
 
-      expect(wrapper.emitted('change')).toEqual([[{ name: 'test', externalUrl: '' }]]);
-    });
+      beforeEach(() => {
+        name = wrapper.findByLabelText('Name');
+      });
 
-    it('should validate that the name is required', async () => {
-      await name.setValue('');
-      await name.trigger('blur');
+      it('should emit changes to the name', async () => {
+        await name.setValue('test');
+        await name.trigger('blur');
 
-      expect(wrapper.findByText('This field is required').exists()).toBe(true);
-      expect(name.attributes('aria-invalid')).toBe('true');
-    });
-  });
+        expect(wrapper.emitted('change')).toEqual([[{ name: 'test', externalUrl: '' }]]);
+      });
 
-  describe('url input', () => {
-    let url;
+      it('should validate that the name is required', async () => {
+        await name.setValue('');
+        await name.trigger('blur');
 
-    beforeEach(() => {
-      url = wrapper.findByLabelText('External URL');
+        expect(wrapper.findByText('This field is required').exists()).toBe(true);
+        expect(name.attributes('aria-invalid')).toBe('true');
+      });
     });
 
-    it('should emit changes to the url', async () => {
-      await url.setValue('https://example.com');
-      await url.trigger('blur');
+    describe('url input', () => {
+      let url;
+
+      beforeEach(() => {
+        url = wrapper.findByLabelText('External URL');
+      });
 
-      expect(wrapper.emitted('change')).toEqual([
-        [{ name: '', externalUrl: 'https://example.com' }],
-      ]);
+      it('should emit changes to the url', async () => {
+        await url.setValue('https://example.com');
+        await url.trigger('blur');
+
+        expect(wrapper.emitted('change')).toEqual([
+          [{ name: '', externalUrl: 'https://example.com' }],
+        ]);
+      });
+
+      it('should validate that the url is required', async () => {
+        await url.setValue('example.com');
+        await url.trigger('blur');
+
+        expect(wrapper.findByText('The URL should start with http:// or https://').exists()).toBe(
+          true,
+        );
+        expect(url.attributes('aria-invalid')).toBe('true');
+      });
     });
 
-    it('should validate that the url is required', async () => {
-      await url.setValue('example.com');
-      await url.trigger('blur');
+    it('submits when the form does', async () => {
+      await wrapper.findByRole('form', { title: 'environment' }).trigger('submit');
 
-      expect(wrapper.findByText('The URL should start with http:// or https://').exists()).toBe(
-        true,
-      );
-      expect(url.attributes('aria-invalid')).toBe('true');
+      expect(wrapper.emitted('submit')).toEqual([[]]);
     });
   });
 
-  it('submits when the form does', async () => {
-    await wrapper.findByRole('form', { title: 'environment' }).trigger('submit');
-
-    expect(wrapper.emitted('submit')).toEqual([[]]);
+  it('shows a loading icon while loading', () => {
+    wrapper = createWrapper({ loading: true });
+    expect(wrapper.findComponent(GlLoadingIcon).exists()).toBe(true);
   });
 });

spec/frontend/environments/new_environment_spec.js

+import { GlLoadingIcon } from '@gitlab/ui';
 import MockAdapter from 'axios-mock-adapter';
 import { mountExtended } from 'helpers/vue_test_utils_helper';
 import waitForPromises from 'helpers/wait_for_promises';
@@ -39,7 +40,9 @@ describe('~/environments/components/new.vue', () => {
     wrapper.destroy();
   });
 
-  const fillForm = async (expected, response) => {
+  const showsLoading = () => wrapper.find(GlLoadingIcon).exists();
+
+  const submitForm = async (expected, response) => {
     mock
       .onPost(DEFAULT_OPTS.provide.projectEnvironmentsPath, {
         name: expected.name,
@@ -68,10 +71,20 @@ describe('~/environments/components/new.vue', () => {
     expect(input().element.value).toBe(value);
   });
 
+  it('shows loader after form is submitted', async () => {
+    const expected = { name: 'test', url: 'https://google.ca' };
+
+    expect(showsLoading()).toBe(false);
+
+    await submitForm(expected, [200, { path: '/test' }]);
+
+    expect(showsLoading()).toBe(true);
+  });
+
   it('submits the new environment on submit', async () => {
     const expected = { name: 'test', url: 'https://google.ca' };
 
-    await fillForm(expected, [200, { path: '/test' }]);
+    await submitForm(expected, [200, { path: '/test' }]);
 
     expect(visitUrl).toHaveBeenCalledWith('/test');
   });
@@ -79,8 +92,9 @@ describe('~/environments/components/new.vue', () => {
   it('shows errors on error', async () => {
     const expected = { name: 'test', url: 'https://google.ca' };
 
-    await fillForm(expected, [400, { message: ['name taken'] }]);
+    await submitForm(expected, [400, { message: ['name taken'] }]);
 
     expect(createFlash).toHaveBeenCalledWith({ message: 'name taken' });
+    expect(showsLoading()).toBe(false);
   });
 });