feat(popover-edit): Adds support for spanning multiple columns and se… #15724
Conversation
googlebot
added
the
cla: yes
src/cdk-experimental/popover-edit/popover-edit-position-strategy-factory.ts
Outdated
Show resolved
Hide resolved
src/cdk-experimental/popover-edit/popover-edit-position-strategy-factory.ts
Show resolved
Hide resolved
src/cdk-experimental/popover-edit/popover-edit-position-strategy-factory.ts
Outdated
Show resolved
Hide resolved
kseamon
force-pushed
the
popover-edit-mvp-position-experiment
branch
2 times, most recently
from
b341159 to
77694f0
Compare
| @@ -24,19 +36,39 @@ export abstract class PopoverEditPositionStrategyFactory { | |||
| */ | |||
| @Injectable() | |||
| export class DefaultPopoverEditPositionStrategyFactory extends PopoverEditPositionStrategyFactory { | |||
| constructor(protected readonly overlay: Overlay) { | |||
| constructor(protected readonly direction: Directionality, protected readonly overlay: Overlay) { | |||
There was a problem hiding this comment.
The readonly part here seems a little redudant. If somebody decides to change these, they either know what they're doing or they're shooting themselves in the foot.
There was a problem hiding this comment.
I disagree - if someone wants to override these they can do it via the DI system.
Maybe this comes down to philosophy, but IMO readonly and const should be used everywhere that mutability is not a requirement.
src/cdk-experimental/popover-edit/popover-edit-position-strategy-factory.ts
Show resolved
Hide resolved
| } | ||
|
|
||
| private _getOverlayCells(): HTMLElement[] { | ||
| const cell = closest(this.elementRef.nativeElement!, CELL_SELECTOR) as HTMLElement; |
There was a problem hiding this comment.
Maybe we should make closest accept an ElementRef so that we don't have to repeat .nativeElement everywhere. There's also a coerceElement in @angular/cdk/coercion to make it a little easier.
There was a problem hiding this comment.
OTOH, next year when Angular Material drops support for IE11 I'd like to remove closest and use the native closet directly.
|
|
||
| const row = closest(this.elementRef.nativeElement!, ROW_SELECTOR)!; | ||
| const rowCells = Array.from(row.querySelectorAll(CELL_SELECTOR)) as HTMLElement[]; | ||
| const ownIndex = rowCells.indexOf(cell); |
There was a problem hiding this comment.
Since you're only using the array for the indexOf, you could shorten it a bit to Array.prototype.indexOf.call(row.querySelectorAll(CELL_SELECTOR), cell).
There was a problem hiding this comment.
It's also using slice below - my initial implementation did use the Array.prototype methods but @jelbourn said to use Array.from instead.
...cdk-popover-edit-cell-span-vanilla-table/cdk-popover-edit-cell-span-vanilla-table-example.ts
Outdated
Show resolved
Hide resolved
kseamon
force-pushed
the
popover-edit-mvp-position-experiment
branch
3 times, most recently
from
15cb0f0 to
58382c2
Compare
…tting width of the popup based on the size of the cell(s)
kseamon
force-pushed
the
popover-edit-mvp-position-experiment
branch
from
58382c2 to
f3080b4
Compare
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. |
…tting width of the popup based on the size of the cell(s)