Material-ui: [Table] Remove last border-bottom

[frrrt]

If you wrap a Table with a Paper, two borders will be displayed on the bottom of the table. One for the <td>-Elements and one for the Paper-div. This is also the case in some of the examples in the docs: https://material-ui.com/demos/tables/ (Simple Table for instance).

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Only the border from Paper should be displayed for a crisp look.

Current Behavior 😯

The td border is also displayed by default which makes the bottom of the table look too strong or like having some kind of shadow.

Steps to Reproduce 🕹

Here is a working quick-fix solution: https://codesandbox.io/s/03xm815530

const styles = theme => ({
  tableRow: {
    "&:last-child th, &:last-child td": {
      borderBottom: 0,
    },
  },
};

Another possible quick-fix would be inside the theme. Maybe a permanent solution can be derived from this:

    MuiTableRow: {: {
        '&:last-child td': {
          borderBottom: 0,
        },
      },
    },

PS: Thanks for the amazing job on MUI! :-)

[oliviertassinari]

@frrrt Thank you for the suggestion. Did you try to apply it on the documentation website? It breaks the paginations demos. Given the limited impact of the border and that tables are rarely displayed inside a Paper component like in the demo, I tend to think that the best answer is to ignore the problem. What do you think?

[frrrt]

@oliviertassinari I agree with you! The problem is minor and can easily be fixed in case a table is displayed inside a paper (which I happen to do quite often 🤷‍♂️ ).

Regarding the pagination example: I think it would be more logically if the border would not be displayed on the bottom of the last table row but on the top of the table footer instead. (There is also the case when the last table row is empty and spanning multiple rows.) But this is nitpicking.

[dandrei]

I'm also displaying tables inside Paper elements. The following global override when creating the theme works great to remove the bottom-border (leaving just the ones between rows):

const theme = createMuiTheme({
    //
    // other theme stuff
    //

    overrides: {
        MuiTableRow: {
            root: {
                "&:last-child td": {
                    borderBottom: 0,
                },
            }
        }
    },
});

[oliviertassinari]

@dandrei Maybe we could add a new prop for this style, like disableLastBorder.

[dandrei]

Hi @oliviertassinari, I haven't had issues with this after I found the override solution. But if the bottom border is something that many people ask about, it makes sense to have one of the following:

• The solution documented on the /components/tables/ page.
• A new prop that doesn't require the users to add the override.

Either way, it's going to end up on the documentation page.

Not sure what the MUI policy is regarding adding new props. It would be either a boolean (like disableLastBorder) or a variant (in case there are other ways people use row borders).

[oliviertassinari]

@dandrei IMHO, we should let our users upvote this issue before investing time in better addressing the problem, with a new prop or not.

[dandrei]

Agreed.

[mbrookes]

I'd rather we remove the bottom border than add a prop, but that's considered a breaking change, so is on hold for v5.

[oliviertassinari]

What about this diff?

diff --git a/packages/material-ui/src/Table/Table.js b/packages/material-ui/src/Table/Table.js
index 9e791bb3d..098af4744 100644
--- a/packages/material-ui/src/Table/Table.js
+++ b/packages/material-ui/src/Table/Table.js
@@ -23,6 +23,12 @@ export const styles = theme => ({
   stickyHeader: {
     borderCollapse: 'separate',
   },
+  /* Styles applied to the root element if `disableLastBorder={true}`. */
+  disableLastBorder: {
+    '& tbody tr:last-child th, & tbody tr:last-child td': {
+      border: 0,
+    },
+  },
 });

 const Table = React.forwardRef(function Table(props, ref) {
@@ -30,6 +36,7 @@ const Table = React.forwardRef(function Table(props, ref) {
     classes,
     className,
     component: Component = 'table',
+    disableLastBorder = false,
     padding = 'default',
     size = 'medium',
     stickyHeader = false,
@@ -45,7 +52,14 @@ const Table = React.forwardRef(function Table(props, ref) {
     <TableContext.Provider value={table}>
       <Component
         ref={ref}
-        className={clsx(classes.root, { [classes.stickyHeader]: stickyHeader }, className)}
+        className={clsx(
+          classes.root,
+          {
+            [classes.stickyHeader]: stickyHeader,
+            [classes.disableLastBorder]: disableLastBorder,
+          },
+          className,
+        )}
         {...other}
       />
     </TableContext.Provider>
@@ -71,6 +85,10 @@ Table.propTypes = {
    * Either a string to use a DOM element or a component.
    */
   component: PropTypes.elementType,
+  /**
+   * If `true`, remove the border on the last row.
+   */
+  disableLastBorder: PropTypes.bool,
   /**
    * Allows TableCells to inherit padding of the Table.
    */

[mbrookes]

I don't think you read my last comment :)

[oliviertassinari]

What do you mean by removing it? From what I recall, we didn't have enough information to know when the border should be removed. The purpose of the prop is to let the developer provide this information (when they notice it).

[mbrookes]

It was discussed in the associated (closed) PR: #15899.

[oliviertassinari]

@mbrookes Thanks for the link. As far as I can follow the pull request (if we ignore the breaking change), we have explored two different approaches but they had serious limitations:

1. eda86218859448ad11f0cf90c5208eb597db071b had table pagination missing border-bottom, and the virtualization with extra borders
2. 221cc8cd6485566f76241e00bb5828041acdf8b5 was breaking scrollable mobile version

I think that the limitation of the current approach (on master) is the least worth limitation, and with an extra prop, we can make it OK.

[mbrookes]

the virtualization with extra borders

I'd forgotten about that. On that basis, I'd vote we still wait for v5, and make the bottom border opt-in. (Follow the spec by default.)

[oliviertassinari]

I have found a new use case where disableLastBorder would have been valuable:

without

with

[marcosvega91]

It's good to add this simple feature. In same cases I did it manually