perf(a11y): Make modals accessible

[sabicalija]

close #210

[sabicalija]

I've created a PR and added a first draft how I would approach modals. @klues Can you please have a look about the current changes? Opening/closing modals is possible, style needs to be updated, but I wanted to verify/discuss this approach before making changes in modal-related all files.

I've had a look in all modal components. All of them look like this, right?

File: src/vue-components/modals/addMultipleModal.vue

<div class="modal">
  <div class="modal-mask">
    <div class="modal-wrapper">
    <div class="modal-container" @keyup.27="$emit('close')" @keyup.ctrl.enter="save()"> ... </div>
  </div>
</div>

I think we can drop CSS classes 'modal', 'modal-mask', and 'modal-wrapper'.

[klues]

Thanks for your contribution! 👍

I've had a look in all modal components. All of them look like this, right?

correct.

I think we can drop CSS classes 'modal', 'modal-mask', and 'modal-wrapper'.

Yes, currently the CSS is in modal.css. Probably we can drop most of it and what's needed should go to modal.vue.

I like the approach, looks good to me! What we should consider to add to modal.vue:

• "X" to close the modal at the top right
• handlers for Esc for closing/aborting or Enter (or Ctrl+Enter) for saving - here it should be possible to pass a function to be called on save() from the actual modal to the base modal.
• default templates for the modal-footer like a OK/Cancel buttons or maybe even a possibility to define custom buttons by simply passing an array of labels and callback-functions?! I don't know to which extent this makes sense, if it should also be possible to configure things like the two rows of buttons from the "edit element modal" (see below) just by passing parameters to modal.vue?! Something like <modal :btns-footer="[{label: 'Cancel', row: 1, faIcon: 'times', fn: cancelFn}, ...]"/>

Edit Element Modal:

[klues]

For the headers, probably also some generic code makes sense - possibility to simply pass a header text for the default case.

Related to this: a generic modal for replacing the browser-internal confirm() modal would also make much sense - for a more consistent UX:

[sabicalija]

I think we can drop CSS classes 'modal', 'modal-mask', and 'modal-wrapper'.

Yes, currently the CSS is in modal.css. Probably we can drop most of it and what's needed should go to modal.vue.

Yes, I agree.

I like the approach, looks good to me! What we should consider to add to modal.vue:

• "X" to close the modal at the top right

Yes, we can add that. We could do it in two ways, depending on what we want to achive:

• Static 'X': this would be part of the template of modal.vue itself and not inside of a <slot> element. Thus, it won't be able to "overwrite" it. Nevertheless, we could provide the option (e.g. via properties) to display the "X" or not.
• Slot "X": this would be part of the "header" slot for instance, but could be overwritten by providing custom slots and would require a new implementation of "X" if it is still necessary.

• handlers for Esc for closing/aborting or Enter (or Ctrl+Enter) for saving - here it should be possible to pass a function to be called on save() from the actual modal to the base modal.

Do you mean to pass, for instance, two functions like handleEsc and handleEnter to customize functionality of modals? Currently, Esc triggers the closing of <dialog> elements without the need of an implementation to toggle its visibility.

• default templates for the modal-footer like a OK/Cancel buttons or maybe even a possibility to define custom buttons by simply passing an array of labels and callback-functions?! I don't know to which extent this makes sense, if it should also be possible to configure things like the two rows of buttons from the "edit element modal" (see below) just by passing parameters to modal.vue?! Something like <modal :btns-footer="[{label: 'Cancel', row: 1, faIcon: 'times', fn: cancelFn}, ...]"/>

I think it might make sense to implement Cancel button and Ok button, but if further buttons are required (I don't know how often that's the case, and if an (accessible) layout can be defined upfront), I'd suggest to go for custom slots. One would need to define the <button> elements and could register the handlers there directly. At least, if I understand it correctly. Like:

<template>
  <modal>
    ...
     <slot #footer>
       <button @click="handleCancel">Cancel</button>
       <button @click="handleOk">Ok</button>
       <button @click="handleCustomAction1">Custom Action 1</button>
       <button @click="handleCustomAction1">Custom Action 2</button>
     </slot>
  </modal>
</template>

Furthermore, we could provide modal-related CSS classes to allow easier/quicker design and less repetition, e.g.

<button class="modal-btn row-2 ???">Custom Action 1</button>

For standard cases, where only Cancel and OK are required, one could maybe pass handlers like that.

<template>
  <modal :modal-cnl="handleCancel" :modal-ok="handleOk">
    ...
</template>

I think this is a more understandable design when customizing functionalities of modals, rather than passing arrays around. Furthermore, it keeps the modal component cleaner and separates the responsibilities more appropriately. At least, from my current understanding.

Is there a difference between the handleEsc and handleSave discussion before or are handleCancel and handleOk alternative names describing the same thing?

Edit Element Modal:

[sabicalija]

For the headers, probably also some generic code makes sense - possibility to simply pass a header text for the default case.

Related to this: a generic modal for replacing the browser-internal confirm() modal would also make much sense - for a more consistent UX:

Yes, I think both suggestions are really good and make sense! I guess, the header text, like "Edit grid item" from the previous screenshot should be passed as a parameter to the modal. Right?

[klues]

Static 'X': this would be part of the template of modal.vue itself and not inside of a element.

I would opt for this. I think almost every modal should have the same possibility to close it. An exception maybe would be modals which require some user input, but I don't think we have something like this until now ("confirm" modals should evaluate "X" as "cancel"). But if we need it, I would simply hide the "X" via a property.

Do you mean to pass, for instance, two functions like handleEsc and handleEnter to customize functionality of modals? Currently, Esc triggers the closing of elements without the need of an implementation to toggle its visibility.

OK, then handleEsc probably isn't needed, we don't have modals where some custom actions must be performed on canceling a modal. And handleEnter/handleSave, we would need to handle the default "save" button in the footer - which probably also should have a default keyboard handler like Enter or Ctrl + Enter.

I think it might make sense to implement Cancel button and Ok button, but if further buttons are required (I don't know how often that's the case, and if an (accessible) layout can be defined upfront), I'd suggest to go for custom slots. One would need to define the elements and could register the handlers there directly. At least, if I understand it correctly. Like:

Yes, currently there's only the edit element modal with 4 buttons, I think all other ones have two buttons. So for now, we can stick with your suggestion and adapt/extend it, if needed at some point in the future.

<modal :modal-cnl="handleCancel" :modal-ok="handleOk">

As said, handleCancel probably isn't needed, but maybe there should be an additional property :modal-ok-label in order to make it possible to override OK with e.g. Save. And then :modal-ok-handler instead of :modal-ok in order to make it clear.

I think this is a more understandable design when customizing functionalities of modals, rather than passing arrays around.

If we would have more modals with many buttons, I would like the idea to not have to think about the layout every time - but a said, if it's only the exception like now, I agree with you.

Is there a difference between the handleEsc and handleSave discussion before or are handleCancel and handleOk alternative names describing the same thing?

Same thing.

Yes, I think both suggestions are really good and make sense! I guess, the header text, like "Edit grid item" from the previous screenshot should be passed as a parameter to the modal. Right?

Correct. But in this case, it will be a custom header, since it also shows a small image and the label of the element you're currently editing. And it reminds me of another thing that would be good in the global modal.vue: some kind of "help" icon next to the "X" (see screenshot above) and a property like :help-location which defines which location is opened at clicking on the icon.

[sabicalija]

Another question came up: should the default modal implementation contain the "X" for closing and a button "Cancel" for canceling? If so, are they any different or do they behave the same? Do we need both?

[klues]

I think both should be part of the default modal implementation, but should result in the same action, both are simply closing the modal without additional actions. I think it makes sense to have both, since some users will search for the "X" at the top and some for the possibility to "cancel" at the buttons. Other implementations like bootstrap are doing it in the same way.

[sabicalija]

I'm wondering about $t. What is it? Where is it loaded? Where's the source?

<div class="modal-footer">
    <div class="button-container">
        <button @click="$emit('close')" :title="$t('keyboardEsc')">
            <i class="fas fa-times"/> <span>{{ $t('cancel') }}</span>
        </button>
        <button @click="save()" :title="$t('keyboardCtrlEnter')">
            <i class="fas fa-check"/> <span>{{ $t('insertWords') }}</span>
        </button>
    </div>
</div>

This snippet is from templateModal.vue. Is this just a template implementation or is it used somewhere?

I've seen that the Cancel and OK button, usually look the same, with almost identical implementation. However, in the templateModal.vue I don't understand why $t("insertWord") is used instead of $t("ok").

This is my current suggestion:

<template>
    <dialog ref="modal">
        <div class="modal-header">
            <span>{{ title }}</span>
            <slot name="header"></slot>
            <button @click="close"><i class="fas fa-times"></i></button>
        </div>
        <div class="modal-body"><slot></slot></div>
        <div class="modal-footer">
            <slot name="footer">
                <slot name="cancel-button">
                    <button @click="close" :title="$t('keyboardEsc')">
                        <i class="fas fa-times">{{ $t('cancel') }}</i>
                        Cancel
                    </button>
                </slot>
                <slot name="ok-button">
                    <button :title="$t('keyboardCtrlEnter')">
                        <i class="fas fa-check">{{ $t('ok') }}</i>
                        Ok
                    </button>
                </slot>
            </slot>
        </div>
    </dialog>
</template>

This way, one add custom content to the header, and we can make title and esc optional by parameters.

Since Cancel and OK are almost the same for all components, it might make sense to allow to make either the whole "footer" or "cancel" and "ok" buttons individually customizable via slots.

Cancel functionality is the same for all components, so it can be part of the modal.vue implementation. However, I don't know about the "ok" button. What does save() do (usually)? Does it make sense to make it part of modal.vue too?

[klues]

Questions answered personally ;)

[sabicalija]

Do you have an idea (a klue 🤓) what of the previous changes might have caused this error message?

mainVue.js:117 TypeError: Cannot read properties of null (reading 'id')
    at Proxy.render (gridEditView.vue?./node_modules/vue-loader/lib/loaders/templateLoader.js??ruleSet%5B1%5D.rules%5B1%5D!./node_modules/vue-loader/lib/index.js??vue-loader-options:211:37)
    at Vue._render (vue.esm.js:2581:28)
    at VueComponent.updateComponent (vue.esm.js:3021:27)
    at Watcher.get (vue.esm.js:4205:33)
    at Watcher.run (vue.esm.js:4281:30)
    at flushSchedulerQueue (vue.esm.js:3267:17)
    at Array.eval (vue.esm.js:3902:20)
    at flushCallbacks (vue.esm.js:3824:18)

[klues]

To which lines oft code does the browser take you, if you click the link in the message in the console?

[sabicalija]

Initially I thought, the error may be caused by some changes in gridEditView.vue, but I've only changed some lines. And the error doesn't seem related.

This is the line. I didn't click on it initially but looked it up manually. But the code loaded in the browser uses different sources..

Still, it's not obvious for me. I'd have to have a more detailed look.

[sabicalija]

This is the entire console output. There is another line causing an error, that I didn't mention before. Maybe it helps..

[klues]

The error seems to happen in gridEditView.vue - so what does the browser show when clicking on this link in the console?

[klues]

this.component = component is the line in MainVue.js where a view component is loaded via router.js (e.g. gridEditView.vue).

[sabicalija]

To which lines oft code does the browser take you, if you click the link in the message in the console?

This is the relevant line.

Maybe it's caused by the fact that the parent/root component inside of the setNavigationModal.vue component changed from <div> to <modal>?

[klues]

I assume it's this line where the setNavigationModal is included:
https://github.com/asterics/AsTeRICS-Grid/blob/master/src/vue-components/views/gridEditView.vue#L32

The property grid-id is taken from gridData.id and gridData seems to be not defined in your case. Normally v-if="showNavigateModal" should prevent this, since showNavigateModal only gets true after gridData is already loaded.

[sabicalija]

I've refactored the changes with the store, reviewed and refactored the design of baseModal.vue and tested everything (basic).

The previous version, that you've seen before, is at branch alija/feature/modal-prev. The compare of that branch and the current one alija/feature/modal isn't working properly on GitHub, but locally it does. I don't think you need to compare them, looking at the latest version is enough.

I've pushed both branches to the original repository, so you only need to pull these branches and not another repository.

Don't be surprised by the indentation of some commit/components. I've kept it this way so that it's easier to review the changes. Had I corrected the intendation all lines would have been indicated as changed. We can fix this before merging the changes, after review and test.

I've made some notes this time while reviewing everything i implemented before and while testing and understanding the modals better.

I'd recommend you to first have a look on baseModal.vue and modalMixin.js and afterwards read the notes (briefly) and scroll further down to every element individually and read the respective commit in parallel, to understand the changes (and potential issues with some of the changes) that are introduced with this PR.

Notes

• beforeDestroy hooks
  Some modals have beforeDestroy hooks. I was wondering if the current design might be problematic, since beforeDestroy hooks are only rarely executed. Most of the modals stay in the DOM when some views are opened. progressBarModal and searchModal are open throughout the application life cycle if i'm not wrong. Earlier, I guess, it used to be executed every time a modal was removed from the DOM after closing it. These changes should be reviewed in terms of beforeDestroy() hooks and if them not being executed as frequently introduces some problems. A possible solution might be, to just simply change beforeDestroy to a method and execute it on a close event.
  I've noticed that some components revert the help location - I guess the intention was to revert to the last help location, once the modal is closed. If we want to keep it this way, beforeDestroy should be renamed and executed on every close of a modal.
  But before changing anything, I wanted to get your opinion about the current changes and about this issue.
  Reverting to the last help location, could potentially be integrated in the base-modal and removed from all modals.
  Additionally, setting the help location could be integrated in the base-modal as well.

• dragenter, dragover, drop (editElement.vue)
  editElement had some event listeners different from the other modals. i didn't had a close look to the reason for those listeners and if it's still going to work with the current modal design, or if some other changes might be necessary. i guess it best for @klues to judge upon if there might be potential issues and test or changes are necessary.

• v-on, $listeners
  I've added v-on="\$listeners" to all modals. You'll see it being used in every modal. This allows the parent to register event listeners that the base-modal is emitting without the need for the child modal component to handle listening/emitting itself.

• Style classes from modal.css are part of base-modal now. I think this is a viable solution, some styles are not scoped but global and thus available/usable in other components and their childs, using base-modal. They could be moved from base-modal (to keep it clean) and moved to the components where they are actually used. But I can't tell right now, since I don't know all the code.

• v-focus, update hook, not working properly, what was the intention?
  The v-focus update hook had issues and i had to remove it almost everywhere because it introduced a lot of issues. basically, input element with it, would be focused every time they got updated. The previous implementation was like this:

  Vue.directive('focus', {
          inserted: function (el, binding) {
              if (binding.value || binding.value === undefined) {
                  if (el.focus) el.focus();
                  if (el.select) el.select();
              }
          },
          updated: function (el, binding) {
              if (binding.value) {
                  if (el.focus) el.focus();
                  if (el.select) el.select();
              }
          }
      });

  The name for the hook is not updated but update. After correcting and verifying it's correctness, i noticed that this implementation of the update hook might be problematic, since it basically focuses an element every time it's updated. i don't think that was the intention behind it. it never made issues before, because it never was executed in the first place, because of the wrong hook name.

  Now, all modals behave the same way, by focusing the first focusable element, when the modal is opened. This is builtin functionality of browser, when using showModal API. base-modal's first element is the close button, as recommended by the source you shared with me, allowing (screenreader) users to quickly close the modal again.

• header/footer-extra
  I introduced two "extra" slots for the baseModal. I've introduced these extra slots due to their necessity while working on refactoring some of the modals. They might appear random at first look or not complete. I'd propose to keep them as they are or add some more slot. But adding slots can also happen also at a later point, once there's actually a need for them.

• elementMoveModal, exportModal, ...: help location?
  The elementMoveModal was missing a help location. Other modals using the help service, would set the help location. This component didn't. I've tested it on the currently release, where is succesfully opened the right help location, but I was missing the function call/setup of the location. In other modals, I've found it. I've added it now, but you might want to have a closer look to it to avoid issue, or maybe there's some other mechanism at place and function call somewhere else needs to be removed.

• gridSelector: mounted hook, problematic?
  gridSelector (and potentially other components) has a mounted hook. Earlier, with all the v-if statements, it was executed every time the/a modal was opened. Now, it's only executed once, most of the time. Do we need changes here?

---

Modals

I hope the changes now are suitable. Everything seems to be working fine, however, we should definitely test everything thoroughly. For this, here is a detailed summary of all changes:

I've kept the commits 'clean', by which I mean, all necessary changes to refactor a single modal are in a single commit.
I hope you can review everything just by using the commits. However, below are some notes from my own review, just so we don't miss anything.

exportModal.vue

Parent: allGridsView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.
• I've added a test for grids being set inside currentLanguages to ensure proper initialization.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• Where is the help location set with this modal?
• I've added a call to set the help location according to the help location of the current release.

exportPdfModal.vue

Parent: allGridsView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.

gridLinkModal.vue

Parent: allGridsView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.
• gridTo isn't intialised within data anymore but when the modal is opened (init).

importModal.vue

Parent: allGridsView.vue
Notes:

• Same as exportModal, except from having a mounted hook/init previously.

importDictionaryModal.vue

Parent: dictionariesView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.
• Test for button being disabled is now moved to a computed property (isImportDisabled).

importWordsModal.vue

Parent: dictionariesView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.
• v-focus removed because it isn't working properly with the corrected update hook.
• added an outline around btn-accordion.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• Removed test for aria-hidden in the parent.

addMultipleModal.vue

Parent: gridEditView.vue
Notes:

• mounted hook refactored to init executed every time the modal is opened.
• Reseting inputText upon opening the modal.
• v-focus removed because it isn't working properly with the corrected update hook.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• Usage of helpService unclear. Where can users open the help, set by this component?
• Cleanup unnecessary imports.

editElement.vue

Parent: gridEditView.vue
Notes:

• editElementHeader simplified and added a new slot for baseModal (see also elementMoveModal, setNavigationModal)
• Changed outline for tabs
• Removed v-focus at all child components using it, because it isn't working properly with the corrected update hook.
• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• Hope the approach with the extra buttons in the footer is ok with you.

elementMoveModal.vue

Parent: gridEditView.vue
Notes:

• editElementHeader: see editElement.vue
• gridSelector simplified, hope you are ok with the wide inputs for desktop. This is the first proposal and its simple. But we can revert to previous look&feel.
• mounted hook refactored to init executed every time the modal is opened.
• This component doesn't revert the help location and I don't understand why (it's not necessary?).
• FIXME: I've added a call to set the help location to the best of my knowledge, but I think there is an issue. I can change the help location opened with elementMoveModal depending on the actions I do before. Is this working properly (in the current release)?

gridDimensionModal.vue

Parent: gridEditView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• Initialization of gridData and gridHeight are in init now.
• Simplify body content and style.
• v-if tests for gridData to ensure proper function.
• Cleanup unnecessary imports.

gridTranslateModal.vue

Parent: gridEditView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.

setNavigationModal.vue

Parent: gridEditView.vue
Note:

• editElementHeader: see editElement.vue
• gridSelector: see elementMoveModal.vue
• mounted hook refactored to init executed every time the modal is opened.
• FIXME: I've added a call to set the help location to the best of my knowledge, but I think there is an issue. I can change the help location opened with elementMoveModal depending on the actions I do before. Is this working properly (in the current release)? Is this a "shortcut" for editing an element and setting a 'navigation' action?

unlockModal.vue

Parent: gridView.vue
Note:

• Simplified implementation of numpad using CSS grid.
• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• Destroy of keyHandler object and reset of inputPasscode to ensure properly functioning.
• Cleanup unnecessary imports.

directionInputModal.vue

Parent: gridView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• v-focus removed (because it isn't working properly with the corrected update hook and) for consitent modal behavior.

mouseModal.vue

Parent: gridView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• v-focus removed (because it isn't working properly with the corrected update hook and) for consitent modal behavior.

scanningModal.vue

Parent: gridView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• v-focus removed (because it isn't working properly with the corrected update hook and) for consitent modal behavior.

sequentialInputModal.vue

Parent: gridView.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• beforeDestroy should probably be called in a different way to ensure previous behavior.
• v-focus removed (because it isn't working properly with the corrected update hook and) for consitent modal behavior.
• changed openModal of gridView.vue for all input modals.

progressBarModal.vue

Parent: mainVue.js
Note:

• Cleanup unnecessary imports.
• Test for closable in method close unnecessary?

searchModal.vue

Parent: mainVue.js
Note:

• mounted hook refactored to init executed every time the modal is opened.
• v-focus removed because it isn't working properly with the corrected update hook.
• Reset of searchTerm on close and results and overflow when opening (init).
• Minor style updates.
• Scroll when overflow only on search results.

configPreviewDetail.vue

Parent: noGridsPage.vue
Note:

• mounted hook refactored to init executed every time the modal is opened.
• v-focus removed because it isn't working properly with the corrected update hook.
• selectedImage isn't intialised within data anymore but when the modal is opened (init).

[klues]

Thanks again for your work, it's crazy how big this became - I've labelled the issue "good first issue" when I've created it 🤦‍♂️

I've now only had the time to review baseModal.vue, modalMixin.js and editGrid.vue and changed some things there, see new commits in https://github.com/asterics/AsTeRICS-Grid/tree/alija/feature/modal

Apart from cosmetic things:

• I've added a listener to @close of the base modal in the mixin which resets the modal's internal data. Before e.g. opening editModal, switching to a tab, closing the modal and re-open it results in the same opened tab as before. I think everytime opening a modal should be the same, fresh state.
• I've moved the init-logic back to mounted() and only added listening for open of the base modal there. For me this feels a little bit cleaner since the initialization code is still in "mounted" (where everyone expects it) and there is no need for the @open="init" at the top.
• I think you've forgotten the @ok="save" in the editModal
• this.closeModal() now must always be called as last command, because it resets the internal data, see above.
• regarding the @dragenter, @dragover and @drop events of editElement.vue - they are for being able to drop an image in the tab "Image". It still works, so no problem with the changes.
• What's the v-on=$listeners in gridEdit.vue which you've added?

Please review the changes and tell me what you think.

[sabicalija]

Hi klues,

thanks for the review so far. Let's take it step by step.

• Resetting modal internal data

  Generally I like the idea. It's hard for me to tell (before looking at it in more detail) how much the internal logic of components, such as editModal require a 'fresh' state.

  But thinking through it, there are some issues I'm afraid:

  • Users of baseModal need to be aware, that this.closeModal() needs to be called as last as you wrote too. Although it's clear from looking into modalMixin.js, it's still potentially problematic.

  • Some modals currently close the modal without it being the last command, like importModal.vue. Here's an excerpt:

    // importModal.vue
    async save() {
        if (!this.importData || (this.options.resetBeforeImport && !confirm(i18nService.t("doYouWantToDeleteBeforeImporting")))) {
            return;
        }
        this.$emit('close');
        this.closeModal(); // <-- NOTE: modal is closed before the progress bar is shown
        if (this.options.resetBeforeImport) {
            MainVue.showProgressBar(0, {
                header: i18nService.t('importDataFromFile'),
                text: i18nService.t('deletingGrids')
            });
            await dataService.deleteAllGrids();
            await dataService.deleteAllDictionaries();
        }
    }

    How do you think we should deal with that?

    I feel like we should emit event 'open' and 'close' every time a modal is opened/closed. Components using the baseModal can then clearly define what needs to happen when a modal is opened or closed and from looking into the code of a component, the call flow should be obvious.

• mounted vs init

  I was wondering about your approach here. Let's look at the difference:

  mounted() {
      this.baseModal.$on('open', () => {
          this.editElementId = this.editElementIdParam;
          this.initInternal();
          helpService.setHelpLocation('03_appearance_layout', '#edit-modal');
      });
  },

  init() {
      this.editElementId = this.editElementIdParam;
      this.initInternal();
      helpService.setHelpLocation('03_appearance_layout', '#edit-modal');
  },

  The first version is merely a different way of writing @open="init".
  The second version is way clearer in my opinion, otherwise people might mistake the mounted hook for being called several times, when in fact it isn't.

• editModal: forgotten @ok="save"

  Good catch. Yes, I did forget that. Initially, there was no #footer-extra hence I thought about implementing the ok button in the component. Had I implemented cancel and ok button, the ok button would have called save on @click. When I added #footer-extra, I forgot to add @ok="save"

• @dragenter, @dragover and @drop

  Great!

• v-on="$listeners"

  I've explained this in the post before, but here's a hopefully shorter and better explanation:

  Consider following setup:

  <!-- // Parent.vue -->
  <template>
    <Child @close="onClose"></Child>
  </template>

  <!-- // Child.vue -->
  <template>
    <base-modal></base-modal>            <!-- base-modal emits event 'close' -->
  </template>

  When the base-modal emits events like 'close', Child would need to listen to these events and forward them to Parent, in scenarios like this. When you need to listen to several events from base-modal, you need to add even more code.

  Using v-on="$listeners" forwards all events from base-modal to Parent.

  <!-- // Child.vue -->
  <template>
    <base-modal v-on="$listeners"></base-modal>
  </template>

---

The discussion about how or if to reset a modals internal data, and/or to use @open, @close has some more aspects, I tried to address in my previous post too.

• beforeDestroy

  Most of the modals have a beforeDestroy hook. The way modals where handled previously, adding and removing them from the DOM via v-if, resulted in beforeDestroy being called every time a modal was closed. I'd suggest to treat it the same way as the mounted hook. Move the code to a method and call it on @close.

  This way, no functionality shouldn't be impacted and I hope thereby to avoid introducing unwanted behavior. Using @open/@close would be just as it previously was with mounted/beforeDestroy.

• Help location and revert

  Most of the modal components use to have a (custom) help location. Most of them also revert to the last (help) location when beforeDestory is executed (or, in other words, when a modal was closed).

Here's my proposal. Let me know what you think.

• The baseModal should have another property specifying a custom help location.
• Remove property help to toggle help button or use it for the help location.
• If the help location is set, the baseModal shows the help button in the top right corner (like now) and reverts the help location when the modal is closed.

This way, we avoid adding this code (setting help location, reverting) in every component using baseModal and make sure the help location is properly set for every modal.

• The baseModal emits @open and @close and every component using baseModal listens to those events and does what it needs to when a modal is opened/closed.

This way, every modal can customize what needs to happen upon opening and closing individually. It's clear from looking at the methods called for these events what is happening, without looking into other files.
Furthermore, since previously mounted/beforeDestroy where used, switching to @open/@close should be safe and not introduce unwanted or unexpected behavior. 🤞

I've read through the post on StackOverflow: https://stackoverflow.com/a/50854892/9219743

I understand the need to have a default state (i.e. data) when the code runs. We could do it similar to how they explain it in the post and have a initialState function that initializes the component data and run this function on an @open event.