ui: modal infrastructure #94

raynasorint · 2023-09-27T09:30:43Z

Required for the PR #89.

sgotti

@raynasorint Thanks for the PR.

https://vitest.dev/guide/#configuring-vitest says that if you're using vite to just use the existing vite.config.ts and add the related config there.
CI should execute tests.
I'll put the test file in the same directory of the source and with .test.ts extension (i.e src/components/modals/configmationDialog.spec.ts

src/components/modals/baseModal.vue

src/components/secrets.vue

src/components/modals/confirmationDialog.vue

sgotti

@raynasorint Modal usually requires also focus trapping (avoid focusing on elements outside the modal using keys like tab). You can add this in this PR or add a TODO and implement it in another PR. I suggest to use vueuse useFocusTrap composable function.

sgotti · 2023-09-29T07:03:54Z

src/components/modals/baseModal.vue

@@ -0,0 +1,20 @@
+<template>
+  <Teleport to="#modals">
+    <div v-if="show" class="">


Is show here and in confirmationDialog really needed? Isn't enough to put a v-if on the component tag?

sgotti · 2023-09-29T11:18:10Z

src/components/modals/confirmationDialog.vue

+              id="modal-confirm"
+              type="button"
+              class="btn btn-blue font-bold py-2 px-4 rounded"
+              @click="$emit('result', true)"


reuse emitResult.

sgotti · 2023-09-29T11:18:20Z

src/components/modals/confirmationDialog.vue

+              id="modal-cancel"
+              type="button"
+              class="btn btn-gray font-bold py-2 px-4 rounded"
+              @click="$emit('result', false)"


reuse emitResult.

sgotti · 2023-09-29T11:18:25Z

src/components/modals/confirmationDialog.vue

+  emit('result', value);
+}
+const dismissModal = () => {
+  emit('result', false);


reuse emitResult.

sgotti · 2023-09-29T11:23:07Z

src/components/modals/baseModal.vue

+    ><UseFocusTrap :options="options">
+      <div
+        class="fixed inset-0 bg-gray-900 bg-opacity-40"
+        ref="target"


you're using the UseFocusTrap component so the ref isn't used and needed.
So, or you only use the component (https://vueuse.org/integrations/useFocusTrap/#using-component) or you only use the composable (https://vueuse.org/integrations/useFocusTrap/#usage).

sgotti · 2023-09-29T11:23:38Z

src/components/modals/baseModal.vue

+
+const dismissModal = () => {
+  emit('dismiss', false);
+};


you could also add a test on baseModal to test that it works correctly and also focus trap works.

sgotti · 2023-09-29T11:24:10Z

src/components/modals/confirmationDialog.test.ts

+  expect(wrapper.emitted()).toHaveProperty('result');
+  const result = wrapper.emitted('result')!;
+  expect(result).toHaveLength(1);
+  expect(result[0]).toStrictEqual([true]);


you could also add a new test to check the correct handling of baseElement dismiss.

sgotti

@raynasorint Looks like confirmation dialog disappeared. The new basemodal is instead a confirmation dialog... I'm missing something.

sgotti · 2023-10-05T14:54:41Z

src/components/modals/baseModal.test.ts

+/*test('baseModal esc keyboard', async () => {
+  const wrapper = mount(baseModal, {
+    props: { show: true },
+    global: {
+      stubs: {
+        teleport: true,
+      },
+    },
+  });
+  // Simulate Escape key press
+  await wrapper.trigger('keydown.esc');
+
+  const dismissEvent = wrapper.emitted('result')!;
+  expect(dismissEvent).toBeCalled();
+  // expect(dismissEvent).toBeTruthy();
+  expect(dismissEvent[0]).toEqual([false]);
+  // expect(dismissEvent[0][0]).toBe(false);
+});*/


Why this test is commented out?

Yes, the confirmationDialog was removed due some problems with our tests.
I had difficulty with esting the focus trap in more modals.
I prefered to use only baseModal for now, including useConfirmDialog().
I removed the useless line too and export in cost the repeated code.

Please restore both dialogs since:

they were logically better

Changing an implementation because tests don't work isn't the right thing to do.

Additionally baseModal is not a "base modal anymore" but a confirmation dialog.

If you have issues with some tests push also them so I can take a look.

Ok. I restored them.
@sgotti I also pushed. Thank you

src/components/modals/baseModal.test.ts

raynasorint · 2023-10-10T08:49:00Z

@sgotti I merged your pr,
Thank you

sgotti · 2023-10-10T08:51:51Z

@sgotti I merged your pr,

I don't know what you mean by "merging my pr" but your branch is totally messed up.

You should use git rebase to rebase your commit ON TOP of the current master branch.

raynasorint · 2023-10-10T10:09:18Z

@sgotti I merged your pr,

I don't know what you mean by "merging my pr" but your branch is totally messed up.

You should use git rebase to rebase your commit ON TOP of the current master branch.

Sorry my bad. I've finished to rebase now.

sgotti · 2023-10-10T11:25:29Z

src/components/modals/baseModal.vue

+  tabbableOptions: {
+    displayCheck: 'none',
+  },


Can you provide some details on why this option has been defined?

I added this option to fix an error in my coverage test. The error message was the following:
Error: Your focus-trap must have at least one container with at least one tabbable node in it at all times.
activate node_modules/.pnpm/@vueuse+integrations@9.13.0_focus-trap@7.5.3_universal-cookie@4.0.4_vue@3.2.47/node_modules/@vueuse/integrations/useFocusTrap/component.mjs:11:41
9| let trap;
10| const target = ref();
11| const activate = () => trap && trap.activate();

You should only activate this option in tests by mocking it in some ways. See https://github.com/focus-trap/tabbable#testing-in-jsdom

I tried to mock the test but didn't work anyway.

There are many ways to do this. A simple one could be:

diff --git a/src/components/modals/baseModal.vue b/src/components/modals/baseModal.vue index 8ffb8d6..211023c 100644 --- a/src/components/modals/baseModal.vue +++ b/src/components/modals/baseModal.vue @@ -20,10 +20,16 @@ const options: UseFocusTrapOptions = { onDeactivate: () => { emit('dismiss', false); }, - tabbableOptions: { - displayCheck: 'none', - }, }; + +// tests outside real browsers like jsdom require tabbableOptions.displayCheck +// set to none (https://github.com/focus-trap/tabbable#testing-in-jsdom) +if (import.meta.env.TEST) { + options.tabbableOptions = { + displayCheck: 'none', + }; +} + const emit = defineEmits<{ (e: 'dismiss', value: boolean): void; }>();

@sgotti somewhere where ? To the same pr?

Create a new branch on your fork and push a commit with the changes that cause issues.

Done!
https://github.com/raynasorint/agola-web/tree/ui--modal-infrastrucure-test-issue
Thank you

the call to it(... never executed the test since you wrapped it by an async function that's never awaited. So removing it(...) made the test run and it failed because it wasn't correct.

This is a working test

import { mount } from '@vue/test-utils'; import baseModal from './baseModal.vue'; test('baseModal', async () => { const wrapper = mount(baseModal, { props: { show: true }, global: { stubs: { teleport: true, }, }, attachTo: document.body, }); // wait for focus trap to be ready await wrapper.vm.$nextTick(); await wrapper.find('[tabindex="0"]').trigger('keydown', { key: 'Escape' }); expect(wrapper.emitted().dismiss).toHaveLength(1); expect(wrapper.emitted().dismiss[0]).toEqual([false]); });

The primary issue was that the component by default isn't attached to the document so focus trap wasn't receiving the key events.

Also note the use of expect(wrapper... like explained in the vitest docs to avoid the 'non-null assertion' that's reported as a warning by eslint. You should do the same also in the confirmationDialog tests.

@sgotti thank you so much for the help.
I tried to add the nextTrick() function but I didn't attached the primary component so I think that was the problem.

I put the correct code now.

Thanks

src/components/modals/baseModal.test.ts

src/components/modals/confirmationDialog.test.ts

sgotti requested changes Sep 27, 2023

View reviewed changes

src/components/modals/baseModal.vue Outdated Show resolved Hide resolved

raynasorint force-pushed the ui--modal-infrastructure branch 2 times, most recently from af0017b to 57ccfbf Compare September 28, 2023 08:05

raynasorint changed the title ~~[WIP] ui: modal infrastructure~~ ui: modal infrastructure Sep 28, 2023

raynasorint requested a review from sgotti September 28, 2023 08:09

sgotti reviewed Sep 28, 2023

View reviewed changes

src/components/secrets.vue Outdated Show resolved Hide resolved

raynasorint force-pushed the ui--modal-infrastructure branch from 57ccfbf to d3dcd4e Compare September 28, 2023 08:41

raynasorint requested a review from sgotti September 28, 2023 08:48

sgotti reviewed Sep 28, 2023

View reviewed changes

src/components/modals/confirmationDialog.vue Outdated Show resolved Hide resolved

raynasorint force-pushed the ui--modal-infrastructure branch from d3dcd4e to faa4c21 Compare September 28, 2023 12:03

raynasorint requested a review from sgotti September 28, 2023 12:35

sgotti requested changes Sep 28, 2023

View reviewed changes

src/components/modals/confirmationDialog.vue Outdated Show resolved Hide resolved

src/components/modals/confirmationDialog.vue Outdated Show resolved Hide resolved

src/components/modals/confirmationDialog.vue Outdated Show resolved Hide resolved

raynasorint force-pushed the ui--modal-infrastructure branch from faa4c21 to 1ba65ee Compare September 29, 2023 06:44

raynasorint requested a review from sgotti September 29, 2023 07:05

sgotti requested changes Sep 29, 2023

View reviewed changes

raynasorint force-pushed the ui--modal-infrastructure branch from 1ba65ee to 795f448 Compare September 29, 2023 09:53

sgotti requested changes Sep 29, 2023

View reviewed changes

raynasorint force-pushed the ui--modal-infrastructure branch 4 times, most recently from 44f6534 to 14f61f8 Compare October 5, 2023 13:52

raynasorint requested a review from sgotti October 5, 2023 13:57

sgotti requested changes Oct 5, 2023

View reviewed changes

raynasorint force-pushed the ui--modal-infrastructure branch from 14f61f8 to fa2f9e6 Compare October 5, 2023 15:06

raynasorint requested a review from sgotti October 5, 2023 15:35

raynasorint force-pushed the ui--modal-infrastructure branch 2 times, most recently from e9b186d to c9951fb Compare October 10, 2023 08:01

raynasorint force-pushed the ui--modal-infrastructure branch from c9951fb to 6580633 Compare October 10, 2023 09:41

sgotti reviewed Oct 10, 2023

View reviewed changes

raynasorint force-pushed the ui--modal-infrastructure branch 4 times, most recently from 14219d5 to 19e0b45 Compare October 11, 2023 12:46

raynasorint requested a review from sgotti October 11, 2023 12:47

raynasorint force-pushed the ui--modal-infrastructure branch from 19e0b45 to 8774cc5 Compare October 11, 2023 12:47

sgotti requested changes Oct 11, 2023

View reviewed changes

src/components/modals/baseModal.test.ts Show resolved Hide resolved

src/components/modals/confirmationDialog.test.ts Outdated Show resolved Hide resolved

src/components/modals/confirmationDialog.test.ts Outdated Show resolved Hide resolved

ui: modal infrastructure

fb6aaa3

raynasorint force-pushed the ui--modal-infrastructure branch from 8774cc5 to fb6aaa3 Compare October 11, 2023 13:28

sgotti approved these changes Oct 11, 2023

View reviewed changes

sgotti merged commit a44a1af into agola-io:master Oct 11, 2023

sgotti mentioned this pull request Feb 15, 2024

Ui: Implement a modal infrastructure #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: modal infrastructure #94

ui: modal infrastructure #94

raynasorint commented Sep 27, 2023 •

edited

Loading

sgotti left a comment

sgotti left a comment

sgotti Sep 29, 2023 •

edited

Loading

sgotti Sep 29, 2023

sgotti Sep 29, 2023

sgotti Sep 29, 2023

sgotti Sep 29, 2023

sgotti Sep 29, 2023

sgotti Sep 29, 2023

sgotti left a comment

sgotti Oct 5, 2023

raynasorint Oct 5, 2023

sgotti Oct 5, 2023

raynasorint Oct 6, 2023 •

edited

Loading

raynasorint commented Oct 10, 2023

sgotti commented Oct 10, 2023 •

edited

Loading

raynasorint commented Oct 10, 2023

sgotti Oct 10, 2023

raynasorint Oct 10, 2023

sgotti Oct 10, 2023

raynasorint Oct 10, 2023

sgotti Oct 11, 2023

raynasorint Oct 11, 2023

sgotti Oct 11, 2023

raynasorint Oct 11, 2023

sgotti Oct 11, 2023

raynasorint Oct 11, 2023 •

edited

Loading

ui: modal infrastructure #94

ui: modal infrastructure #94

Conversation

raynasorint commented Sep 27, 2023 • edited Loading

sgotti left a comment

Choose a reason for hiding this comment

sgotti left a comment

Choose a reason for hiding this comment

sgotti Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raynasorint Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

raynasorint commented Oct 10, 2023

sgotti commented Oct 10, 2023 • edited Loading

raynasorint commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raynasorint Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

raynasorint commented Sep 27, 2023 •

edited

Loading

sgotti Sep 29, 2023 •

edited

Loading

raynasorint Oct 6, 2023 •

edited

Loading

sgotti commented Oct 10, 2023 •

edited

Loading

raynasorint Oct 11, 2023 •

edited

Loading