Have KModal pass through their 'submit' and 'cancel' events to simplify other modals #5439

jonboiser · 2019-04-29T20:09:09Z

Summary

Changes KModal to have its parent emit 'submit' or 'cancel' events if these listeners have not been set on KModal. The point of this is to remove some boilerplate like @submit="$emit('submit') in the components that wrap the base KModal, and to reduce the amount variations on these event names (e.g. @close, @success, etc.) that appear in the code (in support of future work towards Apply consistent patterns for all modals #5320). This makes the events of something like AssignmentCopyModal the same as the basic KModal.
For some modals, remove some coupling to Vuex (moving towards a full decoupling of modals from vuex so they are more like simple forms or input elements).
Wrap the text in KButton in a span, so it doesn't add extra whitespace when KButton is set to be basic-link
Fix a warning coming from CoreMenu

Reviewer guidance

The simplest change is to delete lines like @submit="$emit('submit') from wrapped KModals, which should just work. The more complicated handlers like @submit="handleSubmit where handleSubmit may also emit or have complicated side-effects may take a little more checking.

References

…

Contributor Checklist

PR process:

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Testing:

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

Reviewer Checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

Since it will also interecpt keyup.esc on the privacy modal

codecov · 2019-04-29T20:34:58Z

Codecov Report

Merging #5439 into release-v0.12.y will increase coverage by 0.01%.
The diff coverage is 25%.

Impacted Files	Coverage Δ
...src/views/FacilityConfigPage/ConfirmResetModal.vue	`100% <ø> (ø)`	⬆️
...src/views/plan/assignments/AssignmentCopyModal.vue	`80% <ø> (ø)`	⬆️
...ore/assets/src/views/buttons-and-links/KButton.vue	`75% <ø> (ø)`	⬆️
.../src/views/ProfilePage/ChangeUserPasswordModal.vue	`9.67% <ø> (+0.58%)`	⬆️
...src/views/ManageContentPage/DeleteChannelModal.vue	`100% <ø> (+40%)`	⬆️
...evice_management/assets/src/views/WelcomeModal.vue	`50% <ø> (+25%)`	⬆️
...ssets/src/views/ClassEditPage/ClassRenameModal.vue	`0% <ø> (ø)`	⬆️
...ns/learn/assets/src/views/ContentCardGroupGrid.vue	`33.33% <ø> (ø)`	⬆️
...h/assets/src/views/plan/GroupMembersPage/index.vue	`0% <ø> (ø)`	⬆️
kolibri/core/assets/src/views/SideNav.vue	`60.41% <ø> (ø)`	⬆️
... and 31 more

kolibri/core/assets/src/views/KModal.vue

This reverts commit ec04c7f.

…-modals

indirectlylit

looks good to me, thank you!

indirectlylit

actually - question:

It looks like in a number of places we might be emitting events twice? E.g. once immediately due to v-on..., and again inside a custom event handler?

indirectlylit · 2019-05-17T23:22:20Z

kolibri/core/assets/src/views/UpdateNotification.vue

@@ -84,7 +85,7 @@
          this.saveDismissedNotification(this.id);
        }
        this.removeNotification(this.id);
-        this.$emit('closeModal');
+        this.$emit('submit');


is it necessary to emit anything here? I think the v-on will do this for you?

indirectlylit · 2019-05-17T23:23:04Z

kolibri/plugins/coach/assets/src/views/plan/GroupsPage/CreateGroupModal.vue

      callCreateGroup() {
        this.formSubmitted = true;
        if (this.formIsValid) {
          this.submitting = true;
          this.createGroup({ groupName: this.name, classId: this.classId }).then(() => {
-            this.$emit('success');
+            this.$emit('submit');


won't this emit submit twice? Once for v-on="$listeners" and once here?

CreateGroupModal only emits 'submit' once. I think if you explicitly set a @submit=customListener, it sort of overwrites the {submit: parentListener} hidden in v-on="$listeners

Interestingly, though, if you remove this $emit, the event from CreateGroupModal does not appear in the devtools, but it seems that it still goes through the GroupsPage, which is not really what I want since i want to wait till the promise is resolved before emitting 'submit'.

I'm just going to remove the v-on="listeners" for the handful of modals that might emit the same event asynchronously.

In the future, I would like to to remove async behavior API calls from modals, and delegate them to the parents of modals. So modals just synchronously emit events and data, basically.

yeah that seems reasonable.

another alternative is for modals that handle their own internal async code, to explicitly emit an event that's not submit or cancel. For example:

.then(() => this.$emit('finished'))

jonboiser · 2019-05-20T20:52:40Z

I've changed this so that every KModal simply re-emits "cancel" and/or "submit". Also added a warning on KModal in case someone shows a button, but doesnt actually register a listener for the button click.

jonboiser added 6 commits April 29, 2019 12:56

In KModal, pass ‘emit’ and ‘close’ events through parent

ec04c7f

Update modals to use passed-through events

c89e7b6

Make SideNav toggle on keyup.esc

0e7dd79

Since it will also interecpt keyup.esc on the privacy modal

Update tests

97492de

Fix CoreMenu warning

c81dbf3

Wrap kbutton label to remove extra whitespace when basic-link

589b2ce

jonboiser added the TODO: needs review Waiting for review label Apr 29, 2019

jonboiser added this to the 0.12.y milestone Apr 29, 2019

jonboiser changed the title ~~Consistent modals~~ Have KModal pass through their 'submit' and 'cancel' events to simplify other modals Apr 29, 2019

indirectlylit reviewed May 3, 2019

View reviewed changes

kolibri/core/assets/src/views/KModal.vue Outdated Show resolved Hide resolved

Revert "In KModal, pass ‘emit’ and ‘close’ events through parent"

444af6e

This reverts commit ec04c7f.

indirectlylit changed the base branch from develop to release-v0.12.y May 10, 2019 05:04

Merge remote-tracking branch 'origin/release-v0.12.y' into consistent…

46f5354

…-modals

jonboiser force-pushed the consistent-modals branch from e11c822 to 46f5354 Compare May 17, 2019 22:30

indirectlylit approved these changes May 17, 2019

View reviewed changes

indirectlylit reviewed May 17, 2019

View reviewed changes

jonboiser added work-in-progress Not ready for review and removed TODO: needs review Waiting for review labels May 18, 2019

jonboiser added 2 commits May 20, 2019 13:33

Update tests

78bb8ed

Add warning if KModal is missing listeners

6c9dd77

jonboiser force-pushed the consistent-modals branch from 63b3d8b to 6c9dd77 Compare May 20, 2019 20:34

Add @cancel handlers

d395ac6

jonboiser added the TODO: needs review Waiting for review label May 20, 2019

indirectlylit approved these changes May 20, 2019

View reviewed changes

indirectlylit merged commit 7609fda into learningequality:release-v0.12.y May 20, 2019

jonboiser deleted the consistent-modals branch May 21, 2019 17:03

jonboiser removed TODO: needs review Waiting for review labels Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have KModal pass through their 'submit' and 'cancel' events to simplify other modals #5439

Have KModal pass through their 'submit' and 'cancel' events to simplify other modals #5439

jonboiser commented Apr 29, 2019 •

edited

Loading

codecov bot commented Apr 29, 2019 •

edited

Loading

indirectlylit left a comment

indirectlylit left a comment

indirectlylit May 17, 2019

indirectlylit May 17, 2019

jonboiser May 17, 2019

jonboiser May 17, 2019 •

edited

Loading

indirectlylit May 17, 2019 •

edited

Loading

jonboiser commented May 20, 2019

Have KModal pass through their 'submit' and 'cancel' events to simplify other modals #5439

Have KModal pass through their 'submit' and 'cancel' events to simplify other modals #5439

Conversation

jonboiser commented Apr 29, 2019 • edited Loading

Summary

Reviewer guidance

References

Contributor Checklist

Reviewer Checklist

codecov bot commented Apr 29, 2019 • edited Loading

Codecov Report

indirectlylit left a comment

Choose a reason for hiding this comment

indirectlylit left a comment

Choose a reason for hiding this comment

indirectlylit May 17, 2019

Choose a reason for hiding this comment

indirectlylit May 17, 2019

Choose a reason for hiding this comment

jonboiser May 17, 2019

Choose a reason for hiding this comment

jonboiser May 17, 2019 • edited Loading

Choose a reason for hiding this comment

indirectlylit May 17, 2019 • edited Loading

Choose a reason for hiding this comment

jonboiser commented May 20, 2019

jonboiser commented Apr 29, 2019 •

edited

Loading

codecov bot commented Apr 29, 2019 •

edited

Loading

jonboiser May 17, 2019 •

edited

Loading

indirectlylit May 17, 2019 •

edited

Loading