Skip to content

feat(cdk-experimental/table-scroll-container): Create directive and demo #21200

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
Merged
merged 1 commit into from
Jan 8, 2021
Merged

feat(cdk-experimental/table-scroll-container): Create directive and demo #21200

merged 1 commit into from
Jan 8, 2021

Conversation

kseamon
Copy link
Collaborator

@kseamon kseamon commented Dec 2, 2020

Adds the CdkTableScrollContainer and some hooks in CdkTable for it.

For Webkit/Blink browsers, this sizes the scroll bars in coordinating with
sticky rows/columns in the table to create the user experience that a lot of
commenters in #5885 seemed to want but without the accessibility problems of
other approaches.

image

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 2, 2020
@kseamon kseamon force-pushed the table-scroll-container branch from e4ee71d to ff87d6c Compare December 2, 2020 22:33
@jelbourn jelbourn requested review from jelbourn and crisbeto December 2, 2020 22:35
crisbeto
crisbeto reviewed Dec 3, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Aside from the other comments, I'm not sure whether it needs a new top-level package for a single component. Couldn't we have it in a separate NgModule that is exported from the table package?

@kseamon
Copy link
Collaborator Author

kseamon commented Dec 3, 2020

Aside from the other comments, I'm not sure whether it needs a new top-level package for a single component. Couldn't we have it in a separate NgModule that is exported from the table package?

Jeremy wanted this to be in experimental initially, but I'd be fine with that once we've committed to it.

@jelbourn jelbourn requested a review from mmalerba December 3, 2020 23:44
jelbourn
jelbourn reviewed Dec 4, 2020
View reviewed changes
src/cdk-experimental/table-scroll-container/table-scroll-container.ts Outdated
}
}

stickyColsUpdated(sizes: (number|null|undefined)[]): void {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to name these methods e.g. updateStickyColumnsSizes- actively performing the update rather than naming as a handler for the columns being updated.

(also writing out "columns" instead of "cols")

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 was thinking that MatTable is responsible for updating the actual sizes of the columns - the scroll container is reactive to that by updating its own CSS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed Cols to Columns

@kseamon kseamon force-pushed the table-scroll-container branch 9 times, most recently from db1c83d to 5569b47 Compare December 21, 2020 19:31
@kseamon kseamon marked this pull request as ready for review December 21, 2020 19:52
@kseamon kseamon requested review from andrewseguin, devversion and a team as code owners December 21, 2020 19:52
@kseamon
Copy link
Collaborator Author

kseamon commented Dec 21, 2020

Does anyone know how to update the list of allowed scopes for the commit message lint test?

@crisbeto
Copy link
Member

It's in https://github.com/angular/components/blob/master/.ng-dev/commit-message.ts.

@kseamon kseamon force-pushed the table-scroll-container branch from 5569b47 to b92a8c3 Compare December 21, 2020 21:22
@kseamon
feat(cdk-experimental/table-scroll-container): Create directive and demo
5cd4cd9 
Adds the CdkTableScrollContainer and some hooks in CdkTable for it.
For Webkit/Blink browsers, this sizes the scroll bars in coordinating with
sticky rows/columns in the table to create the user experience that a lot of
commenters in angular#5885 seemed to want but without the accessibility problems of
other approaches.
@kseamon kseamon force-pushed the table-scroll-container branch from b92a8c3 to 5cd4cd9 Compare December 21, 2020 22:13
jelbourn
jelbourn approved these changes Dec 22, 2020
View reviewed changes
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, it would be nice if @andrewseguin could take a look.

Also FYI we're not merging any PRs during the holidays

src/dev-app/table-scroll-container/table-scroll-container-demo-module.ts
import {CdkTableScrollContainerModule} from '@angular/cdk-experimental/table-scroll-container';
import {MatButtonModule} from '@angular/material-experimental/mdc-button';
import {MatButtonToggleModule} from '@angular/material/button-toggle';
/*import {MatTableModule} from '@angular/material-experimental/mdc-table';*/
Copy link
Member

Choose a reason for hiding this comment

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

Commented line

@jelbourn jelbourn added area: cdk/table G This is is related to a Google internal issue action: merge The PR is ready for merge by the caretaker labels Dec 22, 2020
@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Dec 22, 2020
@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Jan 6, 2021
@annieyw annieyw added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 8, 2021
@annieyw annieyw merged commit deead4c into angular:master Jan 8, 2021
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Jan 14, 2021
@kseamon @wagnermaciel
feat(cdk-experimental/table-scroll-container): Create directive and d…
80a1253 
…emo (angular#21200)

Adds the CdkTableScrollContainer and some hooks in CdkTable for it.
For Webkit/Blink browsers, this sizes the scroll bars in coordinating with
sticky rows/columns in the table to create the user experience that a lot of
commenters in angular#5885 seemed to want but without the accessibility problems of
other approaches.
@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 Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@devversion devversion Awaiting requested review from devversion

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.
4 participants
@kseamon @crisbeto @jelbourn @annieyw