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.

Sign up for GitHub

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

Jump to bottom

feat(cdk/table): fixed table layouts #20258

Merged
merged 1 commit into from Sep 2, 2020
Merged

feat(cdk/table): fixed table layouts #20258

merged 1 commit into from Sep 2, 2020

Conversation

MichaelJamesParsons
Copy link
Collaborator

Summary

  • Adds fixedColumnWidths input to the CDK table. When true, native tables will display with table-layout: fixed. No-op for flex tables.
  • Reduces the number of times sticky styles are recalculated for native fixed layout tables and flex tables.

Open question

  • fixedLayout might make a better name for the input. Preferences?

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 10, 2020
@MichaelJamesParsons MichaelJamesParsons added area: cdk/table G This is is related to a Google internal issue labels Aug 10, 2020
andrewseguin
andrewseguin reviewed Aug 14, 2020
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

This doesn't seem to account for changes in the table's width and invalidating the cell widths. E.g. if the table's width depends on the screen width, and the window is resized, the cell widths should be invalidated.

src/cdk/table/sticky-styler.ts Outdated Show resolved Hide resolved
src/cdk/table/table.scss Outdated Show resolved Hide resolved
src/cdk/table/table.ts Outdated Show resolved Hide resolved
@MichaelJamesParsons MichaelJamesParsons force-pushed the table-fixed-column-widths branch 5 times, most recently from 70f048e to 82302c5 Compare August 17, 2020 18:02
@MichaelJamesParsons
Copy link
Collaborator Author

@andrewseguin Ready for another review.

@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Aug 19, 2020
jelbourn
jelbourn reviewed Aug 19, 2020
View reviewed changes
src/cdk/table/table.ts Outdated
protected readonly _viewRepeater: _ViewRepeater<T, RenderRow<T>, RowContext<T>>) {
protected readonly _viewRepeater: _ViewRepeater<T, RenderRow<T>, RowContext<T>>,
// Optional for backwards compatibility, but a view ruler will always be provided.
private readonly _viewportRuler: ViewportRuler) {
Copy link
Member

Choose a reason for hiding this comment

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

Adding non-optional constructor params here would be a breaking change since we expect that people extend CdkTable. Can we make this work with these new params being @Optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

src/cdk/table/table.ts
@@ -188,8 +189,10 @@ export interface RenderRow<T> {
selector: 'cdk-table, table[cdk-table]',
exportAs: 'cdkTable',
template: CDK_TABLE_TEMPLATE,
styleUrls: ['table.css'],
Copy link
Member

Choose a reason for hiding this comment

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

Do these styles get included when using mat-table?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the PR to include styles for both the material table and MDC table.

andrewseguin
andrewseguin reviewed Aug 20, 2020
View reviewed changes
src/cdk/table/table.ts
return this._fixedLayout;
}
set fixedLayout(v: boolean) {
this._fixedLayout = coerceBooleanProperty(v);
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be an extremely uncommon edge case, but would it be correct to reset any values here depending on whether the table is being set one way or the other? E.g. when this changes, should _forceRecalculateCellWidths be set to true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense. Yes, both _forceRecalculateCellWidths and _stickyColumnStylesNeedReset should be set to true.
Done

src/cdk/table/table.ts Outdated

// Table cell dimensions may change after resizing the window. Signal the sticky styler to
// refresh its cache of cell widths the next time sticky styles are updated.
this._viewportRuler.change().subscribe(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, any subscribe should be matched with a corresponding unsubscribe somewhere else, otherwise it'll get stuck in memory.

You can use this pipe to handle it: .pipe(takeUntil(this._onDestroy)), just like we do with the dataStream in _observeRenderChanges

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@MichaelJamesParsons MichaelJamesParsons force-pushed the table-fixed-column-widths branch 3 times, most recently from 9d80892 to 6ab9fb1 Compare August 23, 2020 02:52
@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Aug 24, 2020
@mmalerba mmalerba added the target: minor This PR is targeted for the next minor release label Sep 2, 2020
@mmalerba mmalerba merged commit 58e0c48 into angular:master Sep 2, 2020
mmalerba pushed a commit to mmalerba/components that referenced this pull request Sep 5, 2020
@MichaelJamesParsons @mmalerba
feat(cdk/table): fixed table layouts (angular#20258)
c5905be
@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 Oct 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@andrewseguin andrewseguin andrewseguin approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: cdk/table cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MichaelJamesParsons @jelbourn @andrewseguin @googlebot @mmalerba