[docs] Hide duplicate table borders #20809
Conversation
Details of bundle changes.Comparing: 9e2cbae...6d03566 Details of page changes
|
oliviertassinari
changed the title
oliviertassinari
added
bug 🐛
|
I find an issue. We have a problem with rowspan. In this case the row is not the last.
|
@marcosvega91 Great observation, yeah, I would be eager to push this one forward :) |
|
I'm going to stick to my position that the bottom border should be opt-in, rather than opt-out. Given that we're setting sights on v5, I'd suggest that adding a prop now that would soon need to be deprecated is not a such a good idea. (/r/roastme) |
@mbrookes How do you envision the solution in the opt-in paradigm? Does it go against the default border the Material Design specification has?
@eps1lon I think that it depends on the frequency of the need for it. But yeah, if we assume that we will have the system props available in v5, I would envision this approach in the demos: <TableCell css={{ border: 0 }} />
I would be careful with the performance implication of multiple wrappers. So far, we have seen that the performance with the list can be greatly degraded when pushing too far in this direction. How much is too far? I think that for MenuItem, we way above the acceptable limit. We have MenuItem > ListItem > ButtonBase > TouchRipple, 4 components inheritance with a x3 factor (forwardRef, withStyles, the component), this results in 12 component mounted, per option instance: #10778. For the DatePicker Calendar, I think that we are above the acceptable limit too, we have started to explore the issue in https://github.com/mui-org/material-ui-pickers/issues/1674 cc @dmtrKovalenko. For the table more specifically, the number of components inheritance is limited to one. Still, we had a couple of feedback that it's too slow. Comparing the performance of https://material-ui.com/performance/table-raw/ with https://material-ui.com/performance/table-mui/, it takes about x4 more time to render. I have been wondering about providing a pure CSS version: https://trello.com/c/mVzZC4Dn/1458-table-version-full-css-for-performance to completely address the performance concern: <Table striped bordered hover>
<thead>
<tr>
<th>#</th>
<th>First Name</th>
<th>Last Name</th>
<th>Username</th>
</tr>
</thead>However, if we assume that people that need to render a lot of data, will go to the DataGrid, and that the DataGrid can't use the TableCell component because it's too slow, then maybe we don't really need to worry about. It opens the door to a new question. If the DataGrid can't use the |
Hm, okay, it seems that the spec has changed a bit since the last discussion (and since MDv2?). Some examples have a bottom border, some have an outline border, some have a paper-like drop-shadow; so who knows? 🤷🏻♂️ |
oliviertassinari
force-pushed
the
feature/Table/hide_last_border
branch
from
413aa33 to
ba05e80
Compare
marcosvega91
force-pushed
the
feature/Table/hide_last_border
branch
from
ba05e80 to
413aa33
Compare
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
feature/Table/hide_last_border
branch
2 times, most recently
from
cccb5af to
d6e0984
Compare
oliviertassinari
dismissed
eps1lon’s stale review
It seems the the use case is frequent enough (as soon as a paper is used) to make an exception and add a simple prop style.
oliviertassinari
force-pushed
the
feature/Table/hide_last_border
branch
from
d6e0984 to
6c14d8c
Compare
There was a problem hiding this comment.
I still don't agree with adding props for every styling use case. This will be unmaintainable.
Nobody engaged with my arguments which is a red flag for me.
Having a dedicated LastCell = styled(TableCell)(styles) should work as well.
I would be careful with the performance implication of multiple wrappers.
I'm not since every benchmark I've seen is negligible especially if we're talking about one-off use cases. If that is the case then I'd like to see the "performance implication" of a one-off use case that now every component has baked in.
|
I'm closing, I think that if there is no consensus, better give up on it. |
|
Well actually we can stick to overriding the demos without any new API, reopening. |
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
feature/Table/hide_last_border
branch
from
6c14d8c to
6d03566
Compare
oliviertassinari
dismissed
eps1lon’s stale review
remove the public API, udpate docs instead
mbrookes
changed the title
fix #14073
I also updated the TableContext because I think that the stickyHeader property was missing