Get rid of the double modals stack #4536

ValJed · 2024-05-14T11:41:08Z

PRO-5818

Summary

Adds pinia
Stores modals and modals logic in a pinia store. Methods are attached to apos.modal accessible in and out of vue components.
Only BC is that apos.modal.stack isn't existing anymore, a method called apos.modal.get() allows to get all modals or one by ID. Not sure users needed to call this apos.modal.stack before.
Fixes the esc that close modals that wasn't working like before.

PR in Cypress-tools:

hasModalCount method has been updated to use apos.modal.get() instead of apos.modal.stack.

PR in document-versions:

Use the store to get the stack. It'll need to be updated with core. If needed we can tweak document-versions to support both?

Tests are green:
https://github.com/apostrophecms/testbed/actions/runs/9176562447

What are the specific steps to test this change?

Everything works as expected. There is just one fix to make for shortcuts that'll come in another PR.

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

linear · 2024-05-14T11:41:11Z

PRO-5818 Get rid of double modals stack array

boutell

Just questions for now.

boutell · 2024-05-14T12:04:07Z

modules/@apostrophecms/modal/ui/apos/components/TheAposModals.vue

@@ -17,6 +17,10 @@
 <script>
 import cuid from 'cuid';
 import AposThemeMixin from 'Modules/@apostrophecms/ui/mixins/AposThemeMixin';
+import { useModalStore } from 'Modules/@apostrophecms/ui/stores/modal';


How much value is a new dependency on Pinia bringing? apos.modal.modals has been visible in the code as a property other modules might try to look at for a while now - can we maintain that interface to reduce backwards compatibility issues?

apos.modal.modals comes from the backend and is static data so we have no reason to put it does not move.

Ohh right, never mind.

ValJed · 2024-05-15T16:16:18Z

modules/@apostrophecms/modal/ui/apos/components/TheAposModals.vue

@@ -6,131 +6,72 @@
    <component
      v-bind="modal.props"
      :is="modal.componentName"
-      v-for="modal in stack"
+      v-for="modal in store.stack"


Getting the stack from the store, dynamic.

ValJed · 2024-05-15T16:18:10Z

modules/@apostrophecms/modal/ui/apos/composables/AposFocus.js

-    if (!elementsToFocus.value.length) {
+  function cycleElementsToFocus(e, elements) {
+    const elems = elements || elementsToFocus.value;
+    if (!elems.length) {


The power of the composable, modals want to pass elements because they come from the store. But in AposButtonSplit it simply uses the provided elementsToFocus from this composable.

ValJed · 2024-05-15T16:36:37Z

modules/@apostrophecms/ui/ui/apos/stores/modal.js

+    });
+  }
+
+  // Returns true if el1 is "on top of" el2 in the


I think this function is never used..
modals use the overlay click event to close themselves.

Look at AposTiptapAnchor.vue:

v-click-outside-element="close"

This is used to edit links in rich text. Needs to keep working, so click-outside-element.js still needs to work, but could be factored to use the store.

ValJed · 2024-05-22T11:51:02Z

modules/@apostrophecms/area/ui/apos/components/AposAreaExpandedMenu.vue

@@ -6,7 +6,6 @@
    @inactive="modal.active = false"
    @show-modal="modal.showModal = true"
    @esc="close"
-    @no-modal="$emit('safe-close')"


This has been removed because people using modals forget to call it.
The safe-close event is emitting in AposModal when it closes.
I kept the event no-modal to avoid BC.

ValJed · 2024-05-22T11:53:39Z

modules/@apostrophecms/command-menu/ui/apos/components/TheAposCommandMenu.vue

-      const modal = apos.modal.getAt(index);
-      const properties = apos.modal.getProperties(modal.id);
+      const modal = this.getAt(index);
+      const properties = this.getProperties(modal.id);


It would still work using apos.modal but more idiomatic way to use store.

ValJed · 2024-05-22T11:54:25Z

modules/@apostrophecms/modal/ui/apos/apps/AposModals.js

-// similar functionality. This is necessary because
-// sometimes Vue removes elements from the DOM before
-// we can examine their relationships.
-function onTopOf(el1, el2) {


Method moved in store. Not sure it's used anymore since we use the overlay click to close a modal.

See click-outside-element.js. Make sure that functionality does not break with this change.

ValJed · 2024-05-22T11:55:21Z

modules/@apostrophecms/modal/ui/apos/apps/AposModals.js

+  apos.modal.getProperties = modalStore.getProperties;
+  apos.modal.onTopOf = modalStore.onTopOf;
+  apos.confirm = modalStore.confirm;
+  apos.alert = modalStore.alert;


We attach methods from the store. it's working well for function (can be used out of vue).
It's not for computed and refs. (That why I added the get function that allow to get all modals or one by ID).

ValJed · 2024-05-22T11:56:36Z

modules/@apostrophecms/modal/ui/apos/components/AposModal.vue

  }
+  store.updateModalData(props.modalId, { modalEl: modalEl.value });


Updating instantiated Modal in the store (from TheAposModals) with data only available in this component. Allows to have only one array of modals.

ValJed · 2024-05-22T12:37:55Z

modules/@apostrophecms/modal/ui/apos/components/AposModal.vue

+
+function onTab(e) {
+  const currentModal = store.get(props.modalId);
+  cycleElementsToFocus(e, currentModal.elementsToFocus);


Only triggered on tab.

ValJed · 2024-05-22T12:39:36Z

modules/@apostrophecms/modal/ui/apos/components/AposModal.vue

@@ -258,21 +264,24 @@ function trapFocus() {
    .map(addExcludingAttributes)
    .join(', ');

-  elementsToFocus.value = [ ...modalEl.value.querySelectorAll(selector) ]
+  const elementsToFocus = [ ...modalEl.value.querySelectorAll(selector) ]


We now store them in the modal in the store directly.

ValJed · 2024-05-22T12:40:35Z

modules/@apostrophecms/modal/ui/apos/components/TheAposModals.vue

-      const properties = {
-        ...this.modals
-          .find(modal => modal.componentName === stackModal.componentName &&
-            modal.props.moduleName === stackModal.props.moduleName)


Logic moved in the store.

boutell · 2024-05-23T12:16:56Z

modules/@apostrophecms/image/ui/apos/components/AposImageRelationshipEditor.vue

@@ -6,7 +6,6 @@
    @inactive="modal.active = false"
    @show-modal="modal.showModal = true"
    @esc="confirmAndCancel"
-    @no-modal="$emit('safe-close')"


I'm glad you found a way to eliminate this, I always considered it a code wart but wasn't sure how it could be eliminated.

boutell · 2024-05-23T12:18:53Z

modules/@apostrophecms/modal/ui/apos/apps/AposModals.js

-// similar functionality. This is necessary because
-// sometimes Vue removes elements from the DOM before
-// we can examine their relationships.
-function onTopOf(el1, el2) {


See click-outside-element.js. Make sure that functionality does not break with this change.

boutell · 2024-05-23T12:20:54Z

modules/@apostrophecms/ui/ui/apos/stores/modal.js

+    });
+  }
+
+  // Returns true if el1 is "on top of" el2 in the


Look at AposTiptapAnchor.vue:

v-click-outside-element="close"

This is used to edit links in rich text. Needs to keep working, so click-outside-element.js still needs to work, but could be factored to use the store.

…cus composable

…attach method to apos.modal

…ctly

…ke shortcuts one)

ValJed · 2024-05-27T10:28:17Z

@boutell The click outside feature is working well.
I just think that the function onTopOf that is used only for modals isn't used anymore because:

The directive isn't attached to any modal DOM element
Modals are close using esc or by clicking on tthe overlay, and each modal has its own overlay so it cannot close a modal that is below the current one.

I keep it for now, I didn't change any behavior about it, and this PR is big enough. But I'll make tests later to see if we can get rid of this onTopOf method.

* main: Get rid of the double modals stack (#4536) Add field access to the reactive document value (#4552)

ValJed self-assigned this May 14, 2024

ValJed marked this pull request as draft May 14, 2024 11:41

ValJed changed the title ~~adds pinia store for modals~~ Gets rid of the double modals stack May 14, 2024

ValJed changed the title ~~Gets rid of the double modals stack~~ Get rid of the double modals stack May 14, 2024

boutell requested changes May 14, 2024

View reviewed changes

ValJed force-pushed the pro-5818-get-rid-of-double-stacks branch from 373b970 to c4b3e17 Compare May 15, 2024 13:00

ValJed commented May 15, 2024

View reviewed changes

boutell self-requested a review May 15, 2024 17:28

ValJed force-pushed the pro-5818-get-rid-of-double-stacks branch 3 times, most recently from 30659c0 to 87a9bbb Compare May 21, 2024 14:29

ValJed commented May 22, 2024

View reviewed changes

ValJed marked this pull request as ready for review May 22, 2024 12:58

boutell requested changes May 23, 2024

View reviewed changes

ValJed added 6 commits May 27, 2024 11:54

adds pinia store for modals

d1a68c9

attaches to apos.modal methods from store

5cecebd

instantiates pinia

5cb17a9

adds methods to modal store

d685237

moves TheAposModals to composition and use new modal store

c3d51cd

makes modal store manage focus system, makes it compatible with useFo…

66c07da

…cus composable

ValJed added 15 commits May 27, 2024 11:54

plugs modal store to command menu

9e71fc2

removes logs

e0e3ee9

fixes lint errors

c60bae8

removes stack from apos.modal.stack

6b15980

uses pinia store in AposDocsManager

dee908b

fixes modalEl unset in onMounted

cb8e849

updates modal store get method to return all modals if no id passed, …

77dc1ae

…attach method to apos.modal

split resolve and remove methods, call safe-close from AposModal dire…

b942d56

…ctly

removes use of manual safe-close emit when defining a modal (automatic)

2a7a25c

gets back no-modal in AposModal for BC and users potential use

58605f3

uses better key event handlers in AposModal

4fba3c0

do not pass modals to TheAposModals since not used

5008400

fixes modal escape event listener (no need for focus)

5055829

adds changelog

1eb95eb

resolves modal when removing it, no more safe-close needed

4d8b3a8

ValJed force-pushed the pro-5818-get-rid-of-double-stacks branch from c5d346a to 4d8b3a8 Compare May 27, 2024 09:54

closes modal when clicking on overlay, even if other modal opened (li…

ce378d7

…ke shortcuts one)

ValJed requested a review from boutell May 27, 2024 10:29

boutell approved these changes May 28, 2024

View reviewed changes

ValJed merged commit 1959ef0 into main May 28, 2024
8 checks passed

ValJed deleted the pro-5818-get-rid-of-double-stacks branch May 28, 2024 13:53

haroun added a commit that referenced this pull request May 28, 2024

Merge branch 'main' into pro-6094-shortcut-display

5bf6f0d

* main: Get rid of the double modals stack (#4536) Add field access to the reactive document value (#4552)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of the double modals stack #4536

Get rid of the double modals stack #4536

ValJed commented May 14, 2024 •

edited

linear bot commented May 14, 2024

boutell left a comment

boutell May 14, 2024

ValJed May 15, 2024

boutell May 15, 2024

ValJed May 15, 2024

ValJed May 15, 2024

ValJed May 15, 2024

boutell May 23, 2024

ValJed May 22, 2024

ValJed May 22, 2024

ValJed May 22, 2024

boutell May 23, 2024

ValJed May 22, 2024

ValJed May 22, 2024

ValJed May 22, 2024

ValJed May 22, 2024

ValJed May 22, 2024

boutell May 23, 2024

boutell May 23, 2024

boutell May 23, 2024

ValJed commented May 27, 2024

		}
		store.updateModalData(props.modalId, { modalEl: modalEl.value });

Get rid of the double modals stack #4536

Get rid of the double modals stack #4536

Conversation

ValJed commented May 14, 2024 • edited

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented May 14, 2024

boutell 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

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

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

ValJed commented May 27, 2024

ValJed commented May 14, 2024 •

edited