feat(popover-edit): experimental popover edit for tables (mvp) #15496

kseamon · 2019-03-15T18:06:06Z

Adds a new set of directives to cdk-experimental that allow editing of content in tables (including cdk-table). Will be expanded with additional functionality and Material-styled versions in coming PRs.

andrewseguin

Looks great, the examples are really helpful to see what it looks like as the end user, thanks for setting it up so we can see it in the demo app.

It would be valuable to create a simple case of using this with the MatTable just to make sure we have the right considerations for API. For example, common.ts contains '.mat-cellas a selector but this is in the CDK directory and should not be aware ofmat`. How do you want to provide that selector to the underlying logic so that it knows what to look out for?

src/cdk-experimental/popover-edit/common.ts

src/cdk-experimental/popover-edit/edit-events.ts

andrewseguin · 2019-03-20T20:04:50Z

src/cdk-experimental/popover-edit/edit-events.ts

+  readonly hovering = new Subject<Element|null>();
+  readonly mouseMove = new Subject<Element|null>();
+
+  protected currentlyEditing: Element|null = null;


Do you anticipate this service being extended, if so, do you have a use case we can track to make sure we're correctly enabling that case?

I don't have anything specific in mind. I could make them private if that's preferred.

I'll be extending a bunch of the directives for the material versions but probably not the services.

src/cdk-experimental/popover-edit/edit-events.ts

src/cdk-experimental/popover-edit/common.ts

src/cdk-experimental/popover-edit/table-directives.ts

src/cdk-experimental/popover-edit/lens-directives.ts

kseamon · 2019-03-20T20:45:01Z

Re: the mat-selectors

I plan to make mat- versions of these that would be used with the mat-table but I figured that there was no harm in making the cdk version of this compatible with the mat table as well in case people have their own ideas about how it should be styled.

Do you think that warrants a separate demo now or can it wait until the mat version?

jelbourn

I got about halfway through, have to run to a meeting now. I'll try to finish afterwards.

FYI if you respond to comments on the "Files changed" tab in GitHub, you can batch your responses instead of sending one email per reply

src/cdk-experimental/popover-edit/edit-events.ts

jelbourn · 2019-03-22T20:39:39Z

src/cdk-experimental/popover-edit/edit-events.ts

+   * Returns an Observable that emits true when the specified element's cell
+   * is editing and false when not.
+   */
+  editingCell(element: Element|EventTarget) {


Can you explicitly declare the return types for functions? Our docs generation tool isn't smart enough to infer the type.

jelbourn · 2019-03-22T20:48:06Z

src/cdk-experimental/popover-edit/edit-events.ts

+   * Returns an Observable that emits true when the specified element's cell
+   * is editing and false when not.
+   */
+  editingCell(element: Element|EventTarget) {


For a follow-up PR: a lot of our APIs that take an element commonly accept ElementRef | Element

src/cdk-experimental/popover-edit/lens-directives.ts

jelbourn · 2019-03-22T21:19:04Z

src/cdk-experimental/popover-edit/lens-directives.ts

+    '(keydown.enter)': 'onEnter(true)',
+    '(keyup.enter)': 'onEnter(false)',
+    '(keyup.escape)': 'onEscape()',
+    '(document:click)': 'onDocumentClick($event)',


Could these handlers be named for what they do rather than when they're called?

Some of the names are still not that descriptive (not sure how to describe what was formerly "onSubmit", really), but gave it a try.

src/cdk-experimental/popover-edit/lens-directives.ts

src/cdk-experimental/popover-edit/polyfill.ts

src/cdk-experimental/popover-edit/table-directives.ts

src/material-examples/cdk-popover-edit-cdk-table/cdk-popover-edit-cdk-table-example.css

jelbourn

I tried it out locally and found a few weird things:

Using the keyboard to trigger the "Close" button seems to close it and then immediately re-open ("Confirm" works, though)
Hovering over the rows in the examples is super jumpy; could that be tamed with some css?

jelbourn · 2019-03-29T22:20:18Z

src/cdk-experimental/popover-edit/constants.ts

+ */
+
+/** Selector for finding table cells. */
+export const CELL_SELECTOR = '.cdk-cell, .mat-cell, td';


Not a concern for now, but before this exits experimental we'd want to remove any references to "mat" from the cdk package, whatever that would mean here. Might want to add a TODO

Longer term, the thing to do is probably to make that selector customizable via some DI config.

I included mat in there on the off chance someone would want to have a more stylistically unopinionated edit for their mat table than what MatPopoverEdit will be.

Selectors have to be completely static because they're resolved at compile time into generated code. We'd have to do something like creating a child class.

Is that true for this case where it's applying the selector imperatively?

Nvm, I didn't follow-through on how it was being used.

jelbourn · 2019-03-29T22:26:03Z

src/cdk-experimental/popover-edit/edit-ref.ts

+import {EditEvents} from './edit-events';
+
+/**
+ * Service used for communication between the form within the edit lens and the


I would just remove the word "Service" from the description

src/cdk-experimental/popover-edit/table-directives.ts

src/material-examples/cdk-popover-edit-cdk-table/cdk-popover-edit-cdk-table-example.html

jelbourn

LGTM

NehalDamania · 2019-04-12T02:55:45Z

Is there any link for the demo?

kseamon · 2019-04-12T15:58:20Z

Since the feature is still experimental, the demo is not part of the material.angular.io demo site yet. You can clone the repository and run gulp serve:devapp to see it.

angular-automatic-lock-bot · 2019-09-10T18:01:47Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

kseamon requested a review from jelbourn as a code owner March 15, 2019 18:06

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 15, 2019

kseamon force-pushed the popover-edit-mvp branch 8 times, most recently from 9aa4c30 to 46ec7c0 Compare March 18, 2019 15:01

kseamon requested a review from mmalerba as a code owner March 18, 2019 15:01

mmalerba requested a review from andrewseguin March 18, 2019 17:59

mmalerba assigned andrewseguin Mar 18, 2019

andrewseguin reviewed Mar 20, 2019

View reviewed changes

kseamon force-pushed the popover-edit-mvp branch 2 times, most recently from 813f514 to b567792 Compare March 21, 2019 18:41

jelbourn reviewed Mar 22, 2019

View reviewed changes

jelbourn reviewed Mar 23, 2019

View reviewed changes

kseamon force-pushed the popover-edit-mvp branch 2 times, most recently from a12027c to 48fcbee Compare March 29, 2019 20:34

jelbourn reviewed Mar 29, 2019

View reviewed changes

kseamon force-pushed the popover-edit-mvp branch 3 times, most recently from d998e7a to 615d401 Compare April 1, 2019 15:39

feat(popover-edit): experimental popover edit for tables (mvp)

16ce33a

kseamon force-pushed the popover-edit-mvp branch from 615d401 to 16ce33a Compare April 1, 2019 16:12

jelbourn approved these changes Apr 1, 2019

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 1, 2019

jelbourn added target: patch This PR is targeted for the next patch release P2 The issue is important to a large percentage of users, with a workaround pr: merge safe labels Apr 1, 2019

mmalerba merged commit 457ff28 into angular:master Apr 3, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(popover-edit): experimental popover edit for tables (mvp) #15496

feat(popover-edit): experimental popover edit for tables (mvp) #15496

kseamon commented Mar 15, 2019

andrewseguin left a comment

andrewseguin Mar 20, 2019

kseamon Mar 20, 2019

kseamon commented Mar 20, 2019

jelbourn left a comment

jelbourn Mar 22, 2019

jelbourn Mar 22, 2019

jelbourn Mar 22, 2019

kseamon Mar 29, 2019

jelbourn left a comment

jelbourn Mar 29, 2019

kseamon Apr 1, 2019 •

edited

jelbourn Apr 1, 2019

kseamon Apr 1, 2019

jelbourn Apr 1, 2019

jelbourn Mar 29, 2019

jelbourn left a comment

NehalDamania commented Apr 12, 2019

kseamon commented Apr 12, 2019

angular-automatic-lock-bot bot commented Sep 10, 2019

feat(popover-edit): experimental popover edit for tables (mvp) #15496

feat(popover-edit): experimental popover edit for tables (mvp) #15496

Conversation

kseamon commented Mar 15, 2019

andrewseguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseamon commented Mar 20, 2019

jelbourn 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

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kseamon Apr 1, 2019 • edited

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

jelbourn left a comment

Choose a reason for hiding this comment

NehalDamania commented Apr 12, 2019

kseamon commented Apr 12, 2019

angular-automatic-lock-bot bot commented Sep 10, 2019

kseamon Apr 1, 2019 •

edited