feat(popover-edit): experimental popover edit for tables (mvp) - #15496
Conversation
googlebot
added
the
cla: yes
9aa4c30 to
46ec7c0
Compare
andrewseguin
left a comment
andrewseguin
left a comment
There was a problem hiding this comment.
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?
| readonly hovering = new Subject<Element|null>(); | ||
| readonly mouseMove = new Subject<Element|null>(); | ||
|
|
||
| protected currentlyEditing: Element|null = null; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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? |
813f514 to
b567792
Compare
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
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
| * Returns an Observable that emits true when the specified element's cell | ||
| * is editing and false when not. | ||
| */ | ||
| editingCell(element: Element|EventTarget) { |
There was a problem hiding this comment.
Can you explicitly declare the return types for functions? Our docs generation tool isn't smart enough to infer the type.
| * Returns an Observable that emits true when the specified element's cell | ||
| * is editing and false when not. | ||
| */ | ||
| editingCell(element: Element|EventTarget) { |
There was a problem hiding this comment.
For a follow-up PR: a lot of our APIs that take an element commonly accept ElementRef | Element
| '(keydown.enter)': 'onEnter(true)', | ||
| '(keyup.enter)': 'onEnter(false)', | ||
| '(keyup.escape)': 'onEscape()', | ||
| '(document:click)': 'onDocumentClick($event)', |
There was a problem hiding this comment.
Could these handlers be named for what they do rather than when they're called?
There was a problem hiding this comment.
Some of the names are still not that descriptive (not sure how to describe what was formerly "onSubmit", really), but gave it a try.
a12027c to
48fcbee
Compare
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
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?
| */ | ||
|
|
||
| /** Selector for finding table cells. */ | ||
| export const CELL_SELECTOR = '.cdk-cell, .mat-cell, td'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is that true for this case where it's applying the selector imperatively?
There was a problem hiding this comment.
Nvm, I didn't follow-through on how it was being used.
| import {EditEvents} from './edit-events'; | ||
|
|
||
| /** | ||
| * Service used for communication between the form within the edit lens and the |
There was a problem hiding this comment.
I would just remove the word "Service" from the description
d998e7a to
615d401
Compare
615d401 to
16ce33a
Compare
jelbourn
added
target: patch
|
Is there any link for the demo? |
|
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. |
|
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. |
6 participants
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.