feat(column-resize): Experimental column resize for mat-table #16114

kseamon · 2019-05-24T20:55:02Z

Plus some incomplete cdk parts.

To come in follow-up PRs:

Support for interoperating with mat-sort-header
Support for persisting and restoring column sizes
Keyboard/accessibility support
Support for touch interfaces

kseamon · 2019-05-24T20:58:20Z

#8312

jelbourn

I got about halfway through before I got too sleepy and decided to go home

src/cdk-experimental/column-resize/column-resize.ts

src/cdk-experimental/column-resize/event-dispatcher.ts

src/cdk-experimental/column-resize/resizable.ts

jelbourn · 2019-05-24T23:49:18Z

src/cdk-experimental/column-resize/resizable.ts

+  protected readonly destroyed = new ReplaySubject<void>();
+  protected readonly document: Document;
+
+  get minPx(): number {


How impractical would it be to support more units for minx/max?

It would ultimately require the ability to convert them into pixels for use while resizing is in progress. I'm not going to do it, but someone probably could.

src/cdk-experimental/column-resize/resizable.ts

src/cdk-experimental/column-resize/resize-strategy.ts

kseamon · 2019-05-31T18:18:19Z

Addressed existing comments and made some progress on getting tests set up. Still not ready to leave draft state yet.

jelbourn

Is it the intent that some additional padding is necessary in the headers when adding the resize handle?

src/cdk-experimental/column-resize/column-resize-directives/column-resize-flex.ts

src/cdk-experimental/column-resize/column-resize-notifier.ts

src/cdk-experimental/column-resize/column-resize.ts

src/cdk-experimental/column-resize/constants.ts

jelbourn · 2019-07-02T21:00:02Z

src/cdk-experimental/column-resize/resize-strategy.ts

+ * The optimally performing resize strategy for flex mat-tables.
+ * Tested against and outperformed:
+ *   CSS selector w/ CSS variable
+ *   Updating all mat-cell nodes


Were these tests something persistent that we could reference in the future, or just something local/transient?

I just ran them locally in the development app with 100 and 500 rows.

I could imagine a unit test that recreates that scenario, but I'd rather not be the one to write it :P

jelbourn · 2019-07-02T21:24:54Z

@crisbeto @andrewseguin PTAL if you have time

narendrasinghrathore · 2019-07-15T09:35:32Z

@jelbourn Is it patched in upcoming updates for mat-table ?
Waiting for this feature badly for mat-table column drag-drop, resize and multiple headers.
Regards

crisbeto · 2019-07-16T19:27:22Z

src/cdk-experimental/column-resize/column-resize-directives/constants.ts

+  TABLE_LAYOUT_FIXED_RESIZE_STRATEGY_PROVIDER,
+];
+export const FLEX_PROVIDERS: Provider[] = [...PROVIDERS, FLEX_RESIZE_STRATEGY_PROVIDER];
+export const HOST_BINDINGS = {


It feels like overkill to have to maintain this in a separate constant.

I'll punt this one up to @jelbourn for adjudication - I like the DRYness of it.

src/cdk-experimental/column-resize/column-resize-directives/column-resize-flex.ts

src/cdk-experimental/column-resize/column-resize.ts

crisbeto · 2019-07-16T19:34:18Z

src/cdk-experimental/column-resize/column-resize.ts

+    this.ngZone.runOutsideAngular(() => {
+      const element = this.elementRef.nativeElement!;
+
+      fromEvent<MouseEvent>(element, 'mouseover').pipe(


Don't we want mouseenter here?

mouseenter isn't very useful for delegated events. This is a single listener for the whole table.

crisbeto · 2019-07-16T19:34:36Z

src/cdk-experimental/column-resize/column-resize.ts

+          takeUntil(this.destroyed),
+          map(event => closest(event.target, HEADER_CELL_SELECTOR)),
+          ).subscribe(this.eventDispatcher.headerCellHovered);
+      fromEvent<MouseEvent>(element, 'mouseleave').pipe(


How are we going to handle this for touch devices?

That is a great question that I need to think more about.

My leading idea at the moment is to show the resize handles when there is focus within the header row (hopefully the user tapping there will trigger that), or failing that, listen for taps within the header row and show them until a tap outside of the row.

Added touch as a follow-up PR todo in the PR description.

crisbeto · 2019-07-16T19:47:02Z

src/cdk-experimental/column-resize/overlay-handle.ts

+
+  private _dragStarted(mousedownEvent: MouseEvent) {
+    const mouseup = fromEvent<MouseEvent>(this.document, 'mouseup');
+    const mousemove = fromEvent<MouseEvent>(this.document, 'mousemove');


Should we handle touch events here? Usually it's much easier to do it up-front than adding it later on.

Yes - I need to do this, but ideally in a follow up PR as this one is quite big already (will add to my to do list in the PR description).

crisbeto · 2019-07-16T19:49:40Z

src/cdk-experimental/column-resize/overlay-handle.ts

+      // If the mouse moved further than the resize was able to match, limit the
+      // movement of the overlay to match the actual size and position of the origin.
+      if (overshot !== 0) {
+        if (overshot < 0 && deltaX < 0 || overshot > 0 && deltaX > 0) {


Could we reuse the drag-drop module from the CDK here? I know that @andrewseguin managed to implement a resizeable sidenav with it which is very similar to what we need to do for the table.

I looked at it when I was designing this and it seemed more straightforward to do it this way. It's been a while, so I don't remember my exact thought process though.

Worthwhile to add a TODO for now, we can revisit when we want to move out of experimental

TODO added at top of file

crisbeto · 2019-07-16T19:57:59Z

src/cdk-experimental/column-resize/resizable.ts

+          overlayY: 'top',
+        }]);
+
+    return this.overlay.create({


I'm not sure I follow why we need an overlay here. Is it just so we can move the handle while dragging?

In my design doc for this (Google-internal, sorry), I cover this. Basically, there's no better way to get the styling right for the drag handle as it has to visually bleed into both the next and previous cells and also go down the entire table while the dragging is happening according to the internal UI spec I'm basing this on (which is an extension of the public Material Design spec that includes things like Datatable resizing and inline edit)

Can you encode that context into a comment in this file?

crisbeto · 2019-07-16T20:01:44Z

src/cdk-experimental/column-resize/resize-strategy.ts

+  applyMinColumnSize(cssFriendlyColumnName: string, _: HTMLElement, sizeInPx: number): void {
+    const cssSize = coerceCssPixelValue(sizeInPx);
+
+    this._applyProperty(cssFriendlyColumnName, 'min-width', cssSize,


Have you tried this against IE/Edge? They tend to not work very well with min-width/min-height and flexbox.

Honestly I'm not even sure how I'd test that (I only have Macs and Linux boxes here at work). Is this still a concern with Chromium-based Edge?

Probably not, but the Chromium-based Edge is still away from being the default.

crisbeto · 2019-07-16T20:03:28Z

src/cdk-experimental/column-resize/resize-strategy.ts

+    const selector = `.${tableClassName} .${columnClassName}`;
+    const body = propertyKeys.map(key => `${key}:${properties.get(key)}`).join(';');
+
+    this._getStyleSheet().insertRule(`${selector} {${body}}`, index!);


Wouldn't it be simpler to assign the styles directly on the element, rather than having to keep track of a separate stylesheet?

It would be simpler, but I'm doing this as an optimization to avoid doing a dom manipulation for every row in the table.

There'd also be the issue of dealing with rows added later on, which this handles nicely.

andrewseguin · 2019-07-16T21:01:49Z

Change looks good, and the demo works great. Users are going to be excited about this one

My only comment on the PR is that it looks like a lot of the functions and properties are missing JSDocs. I know it's quite a bit to document, but do you mind sweeping through with comments for them?

kseamon · 2020-01-03T16:39:02Z

Working on reviving this at long last. Going to respond to some of the above comments then take on the actionable ones after I get all of this rebased to the modern world.

kseamon · 2020-01-03T17:05:29Z

@jelbourn Re: the padding question

Yes - I think you'd want to have padding inside your cells when using the resize feature. The internal spec has 8px I think.

kseamon

Added a bunch of JSDoc. Please take a look

src/cdk-experimental/column-resize/column-resize-directives/column-resize-flex.ts

src/cdk-experimental/column-resize/column-resize-notifier.ts

src/cdk-experimental/column-resize/column-resize.ts

src/cdk-experimental/column-resize/constants.ts

jelbourn

Took a fresh look at this- just a handful more things I noticed.

Would also be good for @crisbeto and @andrewseguin to take a fresh look

jelbourn · 2020-01-28T20:12:27Z

src/cdk-experimental/column-resize/column-resize-notifier.ts

+  readonly completeImmediately?: boolean;
+}
+
+/** Conduit for resize-releated events within the table. */


Suggested change

/** Conduit for resize-releated events within the table. */

/** Originating source of column resize events within a table. */

jelbourn · 2020-01-28T23:26:50Z

src/cdk-experimental/column-resize/resizable.ts

+          overlayY: 'top',
+        }]);
+
+    return this.overlay.create({


Can you encode that context into a comment in this file?

jelbourn · 2020-01-28T23:29:12Z

src/cdk-experimental/column-resize/resizable.ts

+        this.resizeNotifier.resizeCanceled,
+        this.resizeNotifier.resizeCompleted,
+    ).pipe(takeUntilDestroyed).subscribe(columnSize => {
+      this.elementRef.nativeElement!.classList.remove(OVERLAY_ACTIVE_CLASS);


This method is called _listenForResizeEvents, but this doesn't really describe what happens on resize. Break this part into one or more methods that have a descriptive name?

Re: "Can you encode that context into a comment in this file?", I'm not sure I understand what you're asking me to do.

(nevermind - this was super confusing in the conversation view but I see what you were talking about now)

Split two methods out of _listenForResizeEvents

jelbourn · 2020-01-28T23:32:50Z

src/cdk-experimental/column-resize/resizable.ts

+  }
+}
+
+const OVERLAY_ACTIVE_CLASS = 'cdk-resizable-overlay-thumb-active';


Move this to the top of the file?

jelbourn · 2020-01-28T23:34:41Z

src/cdk-experimental/column-resize/constants.ts

@@ -0,0 +1,16 @@
+/**


How about calling this file selectors.ts?

jelbourn · 2020-01-28T23:39:34Z

src/cdk-experimental/column-resize/overlay-handle.ts

+      // If the mouse moved further than the resize was able to match, limit the
+      // movement of the overlay to match the actual size and position of the origin.
+      if (overshot !== 0) {
+        if (overshot < 0 && deltaX < 0 || overshot > 0 && deltaX > 0) {


Worthwhile to add a TODO for now, we can revisit when we want to move out of experimental

crisbeto

LGTM once all the comments are addressed.

crisbeto · 2020-01-30T20:35:14Z

src/cdk-experimental/column-resize/column-resize.ts

+ * Base class for ColumnResize directives which attach to mat-table elements to
+ * provide common events and services for column resizing.
+ */
+export abstract class ColumnResize implements AfterViewInit, OnDestroy {


This should have a blank @Directive() decorator since it's using Angular features.

crisbeto · 2020-01-30T20:35:34Z

src/cdk-experimental/column-resize/column-resize.ts

+  abstract readonly columnResizeNotifier: ColumnResizeNotifier;
+
+  abstract readonly directionality: Directionality;
+  protected abstract readonly elementRef: ElementRef;


Should be ElementRef<HTMLElement> for stronger typing.

crisbeto · 2020-01-30T20:40:23Z

src/cdk-experimental/column-resize/overlay-handle.ts

+ * Base class for a component shown over the edge of a resizable column that is responsible
+ * for handling column resize mouse events and displaying any visible UI on the column edge.
+ */
+export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {


Should be marked as @Directive.

crisbeto · 2020-01-30T20:41:11Z

src/cdk-experimental/column-resize/overlay-handle.ts

+ * for handling column resize mouse events and displaying any visible UI on the column edge.
+ */
+export abstract class ResizeOverlayHandle implements AfterViewInit, OnDestroy {
+  protected readonly destroyed = new ReplaySubject<void>();


I don't see why this needs to be a ReplaySubject. Since it only ever emits once and completes, it can be a regular Subject.

It's a safety precaution that I've adopted as my default practice - if something subscribes after this is destroyed (perhaps due to some async event or timer) then ReplaySubject will cause it to immediately unsubscribe whereas a normal subject never would.

crisbeto · 2020-01-30T20:41:39Z

src/cdk-experimental/column-resize/column-resize.ts

+ * provide common events and services for column resizing.
+ */
+export abstract class ColumnResize implements AfterViewInit, OnDestroy {
+  protected readonly destroyed = new ReplaySubject<void>();


This should also be a regular Subject.

crisbeto · 2020-01-30T20:42:16Z

src/cdk-experimental/column-resize/overlay-handle.ts

+
+      fromEvent<MouseEvent>(this.elementRef.nativeElement!, 'mouseleave').pipe(
+          takeUntilDestroyed,
+          map(event => event.relatedTarget &&


Have you tested this out in different browsers? I know that IE at least used to have some issues with relatedTarget.

I don't have a good way to test it on IE (I just have access to Mac, Linux, ChomeOS and iOS). I'd appreciate help sorting things like this out before this graduates from experimental.

Windows, IE11, and Edge (non-Chromium) run fine for me on my MBPs via VirtualBox, VMWare Fusion, and Bootcamp. VirtualBox and Bootcamp are free.

crisbeto · 2020-01-30T20:47:02Z

src/cdk-experimental/column-resize/resize-strategy.ts

+  applyMinColumnSize(cssFriendlyColumnName: string, _: HTMLElement, sizeInPx: number): void {
+    const cssSize = coerceCssPixelValue(sizeInPx);
+
+    this._applyProperty(cssFriendlyColumnName, 'min-width', cssSize,


Probably not, but the Chromium-based Edge is still away from being the default.

andrewseguin

LGTM - though in general I would prefer to see more jsdocs for class methods

jelbourn

LGTM

olegkuibar · 2020-03-05T20:49:08Z

@kseamon is there any estimate release date for this ? Thx.

kseamon · 2020-03-05T22:17:37Z

Not yet - I hope to get most of the rest of the coding done in April, but then we will probably wait on some accessibility specs to be complete before moving it out of experimental. No promises, but I don't expect huge api changes so you *could* use it from experimental as of Angular 10 probably.

…

On Thu, Mar 5, 2020 at 3:49 PM Oleg Kuibar ***@***.***> wrote: @kseamon <https://github.com/kseamon> is there any estimate release date for this ? Thx. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16114?email_source=notifications&email_token=AAHBQHJMMQEJSQVPF7NPSDTRGAF4NA5CNFSM4HPSUWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN627PA#issuecomment-595439548>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHBQHOXW2AKUHOLR2QHIY3RGAF4NANCNFSM4HPSUWLQ> .

jelbourn · 2020-03-05T22:19:06Z

The current code is usable from @angular/cdk-experimental, but there's no ETA yet on when this will hit feature completion and stable

angular-automatic-lock-bot · 2020-04-05T00:02:28Z

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.}

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

jelbourn reviewed May 24, 2019

View reviewed changes

kseamon force-pushed the column-resize-almost-mvp branch from 543b8fd to 25043c6 Compare May 31, 2019 18:15

kseamon force-pushed the column-resize-almost-mvp branch from 25043c6 to 753fc20 Compare June 10, 2019 13:46

kseamon force-pushed the column-resize-almost-mvp branch 3 times, most recently from e2ca82c to 472b594 Compare July 1, 2019 18:59

kseamon marked this pull request as ready for review July 1, 2019 18:59

kseamon requested review from devversion and mmalerba as code owners July 1, 2019 18:59

jelbourn reviewed Jul 2, 2019

View reviewed changes

jelbourn requested review from andrewseguin and crisbeto and removed request for devversion and mmalerba July 2, 2019 21:24

crisbeto reviewed Jul 16, 2019

View reviewed changes

kseamon force-pushed the column-resize-almost-mvp branch from 472b594 to e97a7bd Compare January 3, 2020 21:03

kseamon requested a review from a team as a code owner January 3, 2020 21:03

kseamon force-pushed the column-resize-almost-mvp branch from e97a7bd to 0aa2d47 Compare January 3, 2020 21:57

kseamon commented Jan 8, 2020

View reviewed changes

kseamon force-pushed the column-resize-almost-mvp branch from 53e2210 to 2c620fb Compare January 27, 2020 16:34

jelbourn added the target: minor This PR is targeted for the next minor release label Jan 28, 2020

jelbourn reviewed Jan 29, 2020

View reviewed changes

crisbeto approved these changes Jan 30, 2020

View reviewed changes

andrewseguin approved these changes Jan 31, 2020

View reviewed changes

jelbourn approved these changes Feb 6, 2020

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 6, 2020

kseamon added 8 commits February 13, 2020 11:08

feat(column-resize): Experimental column resize for mat-table

6d44d73

Attempt to fix CI failure.

ffd4079

seeing what happens if we remove tsconfig-build

01f9ba0

also for cdk

48dee64

Add a lot of JSDoc

1d173f3

Move takeUntil to the end of pipe chains

d5894bb

review

a150302

review

1d71ac2

kseamon force-pushed the column-resize-almost-mvp branch from 6d44a55 to 1d71ac2 Compare February 13, 2020 16:09

jelbourn merged commit 198911f into angular:master Feb 13, 2020

snyk-bot mentioned this pull request Mar 11, 2020

[Snyk] Upgrade: @angular/cdk, @angular/material, @angular/material-moment-adapter lingounet/testage#9

Merged

This was referenced Mar 21, 2020

[Snyk] Upgrade: @angular/cdk, @angular/material hrueger/AGLight#112

Merged

[Snyk] Upgrade: @angular/cdk, @angular/material hrueger/AGLight#114

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 5, 2020

	/** Conduit for resize-releated events within the table. */
	/** Originating source of column resize events within a table. */

feat(column-resize): Experimental column resize for mat-table #16114

feat(column-resize): Experimental column resize for mat-table #16114

Conversation

kseamon commented May 24, 2019 • edited Loading

kseamon commented May 24, 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

kseamon commented May 31, 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

jelbourn commented Jul 2, 2019

narendrasinghrathore commented Jul 15, 2019 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Jul 16, 2019

kseamon commented Jan 3, 2020

kseamon commented Jan 3, 2020

kseamon left a 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

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

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

Choose a reason for hiding this comment

crisbeto 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

olegkuibar commented Mar 5, 2020

kseamon commented Mar 5, 2020 via email

jelbourn commented Mar 5, 2020

angular-automatic-lock-bot bot commented Apr 5, 2020

kseamon commented May 24, 2019 •

edited

Loading

narendrasinghrathore commented Jul 15, 2019 •

edited

Loading