feat(material-experimental/mdc-table): add mat-table selector #20994

annieyw · 2020-11-10T00:49:00Z

Have consistent selectors between original MatTable and MDC-based MatTable.
Note that the mat-table tag is used for users that are styling tables with flex as per the MatTable documentation: https://material.angular.io/components/table/overview#tables-with-display-flex:~:text=Tables%20with%20display%3A%20flex,-The

crisbeto · 2020-11-10T07:15:09Z

src/material-experimental/mdc-table/table.ts

@@ -16,7 +16,7 @@ import {
 import {_DisposeViewRepeaterStrategy, _VIEW_REPEATER_STRATEGY} from '@angular/cdk/collections';

 @Component({
-  selector: 'table[mat-table]',
+  selector: 'mat-table table[mat-table]',


Shouldn't it be mat-table, table[mat-table]? The current selector is looking for tables inside a mat-table element.
Also it seems like there are other directives that are only scoped to table elements, like MatHeaderCell, MatCell etc. I wonder whether @andrewseguin did this intentionally?

Additionally, the element-based APIs will need additional CSS to align with the flexbox-based style of the older version.

annieyw · 2020-12-03T18:28:49Z

Added selectors for cells and rows
Added CSS for flexbox based styles

src/material-experimental/mdc-table/table.scss

src/material/table/_table-flex-styles.scss

…d tables styles into private mixin

annieyw · 2020-12-07T19:21:34Z

For some reason CI didn't run.
Renamed mixin and added clarifying comments.

crisbeto

LGTM, aside from the one question.

crisbeto · 2020-12-07T19:25:42Z

src/material/table/_table-flex-styles.scss

+$mat-row-horizontal-padding: 24px;
+
+// Only use tag name selectors here since the styles are shared between MDC and non-MDC
+@mixin mat-private-table-flex-styles {


Looking through this again, do you think that we should make these selectors a bit more specific? Currently mat-table {} will match both the MDC and non-MDC elements which could interfere with each other. We could change it to something like mat-table.mat-mdc-table, but I'm not familiar enough to know whether it'll make much of a difference.

The reason for extracting the flex styles into a mixin is so that both MDC and non-MDC table can use the styles

Wouldn't we want the MDC version to be using MDC's styles though? Otherwise it's basically a duplicate of our current table.

MDC only has native table styles. The flexbox-based table is an Angular special.

Wouldn't we still want to make it work using MDC's styles? Not necessarily in this PR, but we can work with the MDC team to split their styles up into more granular mixins so we can apply it to our flexbox table too.

I had been assuming the only styles we were applying exclusively to the flex table were the actual flexbox styles. If there are styles common between them, we should just use the appropriate css classes for those.

jelbourn

LGTM, add merge-ready when ready

src/material-experimental/mdc-table/table.scss

src/material/table/BUILD.bazel

…ex based tables styles into private mixin

* feat(material-experimental/mdc-table): add mat-table selector * fix(material-experimental/mdc-table): extract variables and flex based tables styles into private mixin * fixup! fix(material-experimental/mdc-table): extract variables and flex based tables styles into private mixin (cherry picked from commit f0f3748)

…is initialized The MDC-based table tries to add a class to its internal `tbody` element and it assumes that it's always going to find it. The assumption used to be correct since we only supported native `table` elements for the MDC implementation, however as of angular#20994 we also support flexbox-based ones which throw an error, because they don't have a `tbody`. These changes add a check to prevent the error and include a couple of sanity tests to catch issues like this in the future. I've also reworked the tests so that they use the MDC-based `MatPaginator` and `MatTableDataSource`, instead of the base ones.

…is initialized (#21428) The MDC-based table tries to add a class to its internal `tbody` element and it assumes that it's always going to find it. The assumption used to be correct since we only supported native `table` elements for the MDC implementation, however as of #20994 we also support flexbox-based ones which throw an error, because they don't have a `tbody`. These changes add a check to prevent the error and include a couple of sanity tests to catch issues like this in the future. I've also reworked the tests so that they use the MDC-based `MatPaginator` and `MatTableDataSource`, instead of the base ones.

…is initialized (#21428) The MDC-based table tries to add a class to its internal `tbody` element and it assumes that it's always going to find it. The assumption used to be correct since we only supported native `table` elements for the MDC implementation, however as of #20994 we also support flexbox-based ones which throw an error, because they don't have a `tbody`. These changes add a check to prevent the error and include a couple of sanity tests to catch issues like this in the future. I've also reworked the tests so that they use the MDC-based `MatPaginator` and `MatTableDataSource`, instead of the base ones. (cherry picked from commit 2255b66)

…r#20994) * feat(material-experimental/mdc-table): add mat-table selector * fix(material-experimental/mdc-table): extract variables and flex based tables styles into private mixin * fixup! fix(material-experimental/mdc-table): extract variables and flex based tables styles into private mixin

…is initialized (angular#21428) The MDC-based table tries to add a class to its internal `tbody` element and it assumes that it's always going to find it. The assumption used to be correct since we only supported native `table` elements for the MDC implementation, however as of angular#20994 we also support flexbox-based ones which throw an error, because they don't have a `tbody`. These changes add a check to prevent the error and include a couple of sanity tests to catch issues like this in the future. I've also reworked the tests so that they use the MDC-based `MatPaginator` and `MatTableDataSource`, instead of the base ones.

angular-automatic-lock-bot · 2021-01-29T00:13:58Z

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

annieyw added P2 The issue is important to a large percentage of users, with a workaround G This is is related to a Google internal issue labels Nov 10, 2020

annieyw requested a review from andrewseguin as a code owner November 10, 2020 00:49

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 10, 2020

crisbeto reviewed Nov 10, 2020

View reviewed changes

annieyw force-pushed the mdc-mat-table-selector branch from c929ea1 to 2f399a8 Compare November 13, 2020 21:31

annieyw requested a review from jelbourn November 13, 2020 21:31

annieyw force-pushed the mdc-mat-table-selector branch from 2f399a8 to bb85910 Compare December 2, 2020 19:26

annieyw added the target: patch This PR is targeted for the next patch release label Dec 2, 2020

jelbourn reviewed Dec 4, 2020

View reviewed changes

src/material-experimental/mdc-table/table.scss Outdated Show resolved Hide resolved

annieyw force-pushed the mdc-mat-table-selector branch from 3e6bc2f to dfabefb Compare December 4, 2020 00:45

mmalerba reviewed Dec 4, 2020

View reviewed changes

src/material/table/_table-flex-styles.scss Outdated Show resolved Hide resolved

crisbeto reviewed Dec 4, 2020

View reviewed changes

src/material/table/_table-flex-styles.scss Outdated Show resolved Hide resolved

annieyw force-pushed the mdc-mat-table-selector branch 3 times, most recently from 5c9e15e to 2393c88 Compare December 4, 2020 19:48

annieyw added 2 commits December 7, 2020 10:47

feat(material-experimental/mdc-table): add mat-table selector

9e66fa3

fix(material-experimental/mdc-table): extract variables and flex base…

a7cbbc6

…d tables styles into private mixin

annieyw force-pushed the mdc-mat-table-selector branch from 2393c88 to 391891b Compare December 7, 2020 19:01

crisbeto approved these changes Dec 7, 2020

View reviewed changes

jelbourn approved these changes Dec 7, 2020

View reviewed changes

src/material-experimental/mdc-table/table.scss Outdated Show resolved Hide resolved

src/material/table/BUILD.bazel Outdated Show resolved Hide resolved

fixup! fix(material-experimental/mdc-table): extract variables and fl…

18125b7

…ex based tables styles into private mixin

annieyw force-pushed the mdc-mat-table-selector branch from 391891b to 18125b7 Compare December 7, 2020 20:33

annieyw added the action: merge The PR is ready for merge by the caretaker label Dec 7, 2020

mmalerba merged commit f0f3748 into angular:master Dec 17, 2020

crisbeto mentioned this pull request Dec 23, 2020

fix(material-experimental/mdc-table): error when flexbox-based table is initialized #21428

Merged

annieyw deleted the mdc-mat-table-selector branch December 29, 2020 06:54

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 29, 2021

feat(material-experimental/mdc-table): add mat-table selector #20994

feat(material-experimental/mdc-table): add mat-table selector #20994

Uh oh!

Conversation

annieyw commented Nov 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annieyw commented Dec 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

annieyw commented Dec 7, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Jan 29, 2021

Uh oh!

Uh oh!