Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(table): support sticky headers, footers, and columns #11483

Merged
merged 8 commits into from
Jun 7, 2018

Conversation

andrewseguin
Copy link
Contributor

Closes #5885, #5890

@andrewseguin andrewseguin requested a review from jelbourn May 23, 2018 21:42
@andrewseguin andrewseguin requested a review from amcdnl as a code owner May 23, 2018 21:42
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 23, 2018
@andrewseguin andrewseguin added the target: minor This PR is targeted for the next minor release label May 23, 2018
@@ -59,6 +66,26 @@ export class CdkColumnDef {
}
_name: string;

/** Whether this column should be sticky positioned on the left of the row */
@Input('stickyLeft')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use either start/end or before/after so that the term applies for RTL as well

* sticky value.
* @docs-private
*/
export interface HasStickyState {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of CanStick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM


checkStickyChanged(): boolean;

resetStickyChanged(): void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs descriptions for these other members

* @docs-private
*/
export interface HasStickyState {
/** State of whether the sticky input has changed since it was last checked. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omit "State of"

if (!this._columnsDiffer) {
const columns = (changes['columns'] && changes['columns'].currentValue) || [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would columns not be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sticky input changes, then changes: SimpleChanges will not have the columns property (since it didn't change). Prior to this change, the only time ngOnChange was called was when the columns changed; now this is not the only case.

* the data source provides a new set of data or when a column definition changes its sticky
* input. May be called manually for cases where the cell content changes outside of these events.
*/
updateStickyColumnStyles() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have another method with the same name that's internal; they should have different names if they're doing different things

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, didn't catch that. Kept this as update (it removes/adds) and the internal one as add (it only adds)

return acc || d.checkStickyChanged();
};

if (this._headerRowDefs.reduce(stickyCheckReducer, false)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not just a some?

const hasStickyStateChanged = (d: HasStickyState) => d.checkStickyChanged();

if (this._headerRowDefs.some(hasStickyStateChanged)) {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and I'll add a comment explaining why. The check needs to occur on every definition since it also resets its dirty state

_hasStickyChanged: boolean = false;

/** Whether the sticky value has changed since this was last called. */
checkStickyChanged(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasStickyStateChanged?

});
}

describe('on flex layout', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on "display: flex" table style"

@@ -71,6 +71,12 @@ export class MatFooterCellDef extends CdkFooterCellDef {
export class MatColumnDef extends CdkColumnDef {
/** Unique name for this column. */
@Input('matColumnDef') name: string;

/** Whether this column should be sticky positioned on the left of the right */
@Input() stickyLeft: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vast, vast majority of cases are doing to want to stick to the start(left) side. Optimizing for the common case, I think we should just make the start/left position sticky and the other one stickyEnd.

We should talk separately about whether this should be something like cdkColumnSticky

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, I agree the vast majority wants to just use sticky and mean it to be the start.

crisbeto added a commit to crisbeto/material2 that referenced this pull request May 26, 2018
Based on angular#11483 (comment). Adds a custom tslint rule to enforce that we put getters before setters.
@andrewseguin
Copy link
Contributor Author

Heads up - looks like RTL support is lacking. Left and right are absolute positions and the logic should reverse the column sticky position directions

josephperrott pushed a commit that referenced this pull request May 29, 2018
Based on #11483 (comment). Adds a custom tslint rule to enforce that we put getters before setters.
@ngbot
Copy link

ngbot bot commented May 29, 2018

Hi @andrewseguin! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should capture RTL as well, unless you want to retarget it to a branch

@@ -59,6 +66,26 @@ export class CdkColumnDef {
}
_name: string;

/** Whether this column should be sticky positioned on the start of the row */
@Input('sticky')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument to @Input() is unnecessary when it matches the property name (here and below)

cc @crisbeto might be another fun lint check :)
(also, at some point we should probably think about upstreaming these to codelyzer)

/**
* Mixin to provide a directive with a function that checks if the sticky input has been
* changed since the last time the function was called. Essentially adds a dirty-check to the
* sticky value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include something in the description here that explains why the actual sticky property isn't included in the mixin

* @param stickCellCSS The CSS class that will be applied to every row/cell that has
* sticky positioning applied.
*/
constructor(private isNativeHtmlTable: boolean, private stickCellCSS: string) { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stickCellCSS -> stickCellCss

@andrewseguin
Copy link
Contributor Author

Added two commits:

  • Add support for RTL (directionality changes should influence sticky column positioning)
  • Move sticky setter/getter to the mixin

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I started thinking about this in terms of how tree-shakable the code is. Let's chat on Thursday.

const cellWidths: number[] = this._getCellWidths(rows[0]);
const startPositions = this._getStickyStartColumnPositions(cellWidths, stickyStartStates);
const endPositions = this._getStickyEndColumnPositions(cellWidths, stickyEndStates);
const isLtr = this.direction === 'ltr';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should invert this; it's better to explicitly check for RTL instead of LTR. We've had cases where the check was like this and dir ended up being empty- the component then assumed it was in RTL.

Copy link
Contributor

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

const endPositions = this._getStickyEndColumnPositions(cellWidths, stickyEndStates);
const isRtl = this.direction === 'rtl';

for (let row of rows) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why let vs `const?

return;
}

const numCells = rows[0].children.length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make a pointer to rows[0], its used here and below.

@amcdnl
Copy link
Contributor

amcdnl commented Jun 4, 2018

One footnote of this is its not compatible with virtual scrolling, we would need to rethink some of this when we apply that strategy.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 5, 2018
@tatsujb
Copy link

tatsujb commented Jun 8, 2018

Amazing!!! 😃 Could we have a minor release to test out this feature pretty please? 💐

EDIT : if i'm working straight off of master what sort of syntax should I employ to sticky a column?

@andrewseguin
Copy link
Contributor Author

For examples on how to use the sticky stuff, check out the examples that start with "table-sticky-*"

https://github.com/angular/material2/tree/master/src/material-examples

@jelbourn
Copy link
Member

jelbourn commented Jun 8, 2018

Next minor release is probably going to be in a couple weeks

@tatsujb
Copy link

tatsujb commented Jun 11, 2018

@andrewseguin thanks! I haven't figured out how to have an angular cli boilerplater with master version of material2 though. (tried npm i angular/material2#master got a weird "material2-srcs": "github:angular/material2#master", in pmy package json. which did not load material, i tried adding npm i @angular/material then moving the contents of the material2-srcs in node-modules to @angular/material but as I expected that did not work.)

...sorry I'm a beginner 😢 (I really really want to train up my sticky column use because I plan to replace my hacked-in sticky columns with the official ones as soon as they drop)

@Abhishek-Shrotriya
Copy link

Hi, is there any specific date when the Mat-table release with sticky/ pinned columns for IE be publicly available
Thanks for your help.

@sadplace
Copy link

Hi,

Yes, that is really cool and i really need the sticky columns for my project. So if possible i need to know more a less when this is going to be released. Thanks :)

@tatsujb
Copy link

tatsujb commented Jun 15, 2018

@sadplace three comments higher : #11483 (comment)

@jaya0selva
Copy link

Sticky is not working in IE. In chrome also, Sticky column alignments are breaking while scrolling.

@MrJitu
Copy link

MrJitu commented Jun 21, 2018

Hi, Sticky/Fixed header is not working in IE. Is there any plan to fix this issue in next release?

@Misiu
Copy link

Misiu commented Jun 21, 2018

@andrewseguin are You guys planning to add examples showing sticky header and columns to main material site (https://material.angular.io/)?
Material is awesome and each new feature should he highlighted on main site.
For example look at Kendo UI(https://demos.telerik.com/kendo-ui/) they are clearly showing what's new, what's updated.
Sticky header, footer and columns was one of most desired feature, but when You go to main site there isn't even a single example showing it.

@jelbourn
Copy link
Member

IE11 explicitly won't be supported; we decided that the amount of code it would take to work around the absence of position: sticky (and the resources spent maintaining it) isn't worth it based on its share of browser users.

Andrew was just able to finish up the feature before going out-of-office for a while; I'd be happy to accept PRs that add examples for the docs site.

@Gwindorith
Copy link

Hi!

I have updated to the latest version, but if i add the sticky keyword to multiple columns, the columns go behind each other other then staying at the position where they are.

So all three of my colums end up sticky on the left side of the screen:

`
<th mat-header-cell *matHeaderCellDef mat-sort-header> Resourcegroup
<td mat-cell *matCellDef="let element"> {{element.resourceGroup.resourceGroup}}

    <ng-container matColumnDef="description" sticky>
      <th mat-header-cell *matHeaderCellDef mat-sort-header> Name </th>
      <td mat-cell style="white-space: nowrap; padding-right: 8px;" *matCellDef="let element"> {{element.resourceGroup.description}} </td>
    </ng-container>

    <ng-container matColumnDef="status" sticky>
      <th mat-header-cell class="headerPadding" *matHeaderCellDef> status </th>
      <td mat-cell style="white-space: nowrap; padding-right: 10px;" *matCellDef="let element"> {{element.status}} </td>
    </ng-container>

`

Please advice!

@volei
Copy link

volei commented Jul 6, 2018

I edited, because in the meanwhile found more info and solutions:

I have a similar problem. It seems that it is browser dependant:

  • 2nd and 3rd bottom sticky rows go behind each other with a minimal vertical offset (2px)
    (except for:
    -- 1st footer row. But has clientHeight automatically set to 2,
    -- Footer row with text directly written into HTML DOM, thus not being {{double-curly-brace bound text}}.

  • I found out that in case of only {{}}-text the cdk sets the default clientHeight of the row to 2.

  • My 2 top sticky rows stick ok. Also left and right columns
    - Problem so far only in Chrome (Version 67.0.3396.99 64bit)

  • In Firefox Quantum everything is ok, never hides a sticky row. (even without any solution like below.)

  • I am using native <table cdk-table ...> with <td cdk-footer-cell ...>

My solution No.1: Attach &zwnj; before the {{}}-placeholders (see [https://stackoverflow.com/questions/2812253/invisible-delimiter-for-strings-in-html]). This simulates actual HTML-text.

My solution No. 2: Define height: 20px; or font-size: medium; in CSS or anything similar in cdk-cell, cdk-header-cell, cdk-footer-cell or in the row's CSS-classes. But this is rather painful, if you do not have a comprehensive CSS-strategy.

updated yesterday, July 5th (v.6.3):

  "dependencies": {
    "@angular/animations": "^6.0.7",
    "@angular/cdk": "github:angular/cdk-builds",
    "@angular/common": "^6.0.7",
    "@angular/compiler": "^6.0.7",
    "@angular/core": "^6.0.7",
    "@angular/flex-layout": "^6.0.0-beta.16",
    "@angular/forms": "^6.0.7",
    "@angular/http": "^6.0.7",
    "@angular/material": "github:angular/material2-builds",
    "@angular/platform-browser": "^6.0.7",
    "@angular/platform-browser-dynamic": "^6.0.7",
    "@angular/router": "^6.0.7",
    "core-js": "^2.5.7",
    "npm": "^6.1.0",
    "rxjs": "^6.2.1",
    "zone.js": "^0.8.26"
  },
  "devDependencies": {
    "@angular-devkit/build-angular": "^0.6.8",
    "@angular/cli": "^6.0.8",
    "@angular/compiler-cli": "^6.0.7",
    "@angular/language-service": "^6.0.7",
    "@types/jasmine": "^2.8.8",
    "@types/jasminewd2": "~2.0.3",
    "@types/node": "~8.9.4",
    "codelyzer": "~4.2.1",
    "jasmine-core": "~2.99.1",
    "jasmine-spec-reporter": "~4.2.1",
    "karma": "~1.7.1",
    "karma-chrome-launcher": "~2.2.0",
    "karma-coverage-istanbul-reporter": "^1.4.3",
    "karma-jasmine": "~1.1.1",
    "karma-jasmine-html-reporter": "^0.2.2",
    "protractor": "^5.3.2",
    "ts-node": "~5.0.1",
    "tslint": "~5.9.1",
    "typescript": "~2.7.2"

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Table] Add input for sticky headers