feat(popover-edit): allow tabbing from popup to next/previous cell #15793
Conversation
googlebot
added
the
cla: yes
kseamon
force-pushed
the
popover-edit-spreadsheet-behavior
branch
2 times, most recently
from
5b861bb to
4e20f38
Compare
| } | ||
|
|
||
| private _triggerFormSubmit() { | ||
| this.elementRef.nativeElement!.dispatchEvent(new Event('submit')); |
There was a problem hiding this comment.
Rather than "submitting" the form via the DOM, couldn't we call the submit handler ourselves?
There was a problem hiding this comment.
You could alternatively call .submit() on the form, no?
There was a problem hiding this comment.
I do need the form itself to submit in case the user added any submit handlers.
| } | ||
|
|
||
| protected initFocusTrap(): void { | ||
| this.focusTrap = this.focusEscapeNotifierFactory.create(this.overlayRef!.overlayElement); |
There was a problem hiding this comment.
Should this be cleaned up on destroy?
There was a problem hiding this comment.
The Overlay elements are destroyed, and the focus trap is inside of the overlay bounding box, so I don't think any additional cleanup is needed.
| } | ||
|
|
||
| private _triggerFormSubmit() { | ||
| this.elementRef.nativeElement!.dispatchEvent(new Event('submit')); |
There was a problem hiding this comment.
You could alternatively call .submit() on the form, no?
|
Calling .submit() does not work - It submits the form without calling any event handlers (and triggers an old-school page refresh). |
kseamon
force-pushed
the
popover-edit-spreadsheet-behavior
branch
from
39be059 to
fbe596a
Compare
kseamon
force-pushed
the
popover-edit-spreadsheet-behavior
branch
from
fbe596a to
e3cc445
Compare
| @@ -30,6 +31,12 @@ export class EditEventDispatcher { | |||
| /** A subject that emits mouse move events for table rows. */ | |||
| readonly mouseMove = new Subject<Element|null>(); | |||
|
|
|||
| /** The EditRef for the currently active edit lens (if any). */ | |||
| get editRef(): EditRef<any>|null { | |||
There was a problem hiding this comment.
This could be in a follow-up change, but could we avoid using any here in favor of a generic?
There was a problem hiding this comment.
That would be tricky - the type is generic in the lens (refers to the shape of the form data) but different edits in the same table could have different lenses and this must refer to all of them.
It’d be fine to use unknown here but I don’t know what TS versions we have to support.
There was a problem hiding this comment.
We can make it unknown in a follow-up. As of Angular v8 we support 3.4 as the minimum
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Includes a demo of how this allows a more spreadsheet-like set of user interactions when combined with some of Popover Edit's other features.