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.

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

[Paper] Support outlined Card / Paper #15728

Closed
2 tasks done
mjadczak opened this issue May 16, 2019 · 22 comments · Fixed by #18824
Closed
2 tasks done

[Paper] Support outlined Card / Paper #15728

mjadczak opened this issue May 16, 2019 · 22 comments · Fixed by #18824
Labels
component: card This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@mjadczak
Copy link

The Material spec states that cards on the desktop have 0dp elevation, and should be outlined instead of being delimited using their shadow. I think this variant should be supported.

This has been mentioned in an unrelated issue, but seems to not have been brought back up.

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

Expected Behavior 🤔

There should be a way to get an outlined Card or Paper with 0dp elevation.
This could either take the form of <Card variant="outlined"> (issues - should this automatically set elevation to 0? Should it work with elevation not 0?) or by automatically adopting the style when <Card elevation={0}> is used (issues - does this mean you cannot have a no-outline no-shadow Card?)

I'm not sure whether determining in some global fashion whether we are on "desktop" or not is feasible and whether it should change the default elevation of the card - my instinct is that it should not.

Current Behavior 😯

Currently a Card or Paper with no elevation is flat with no outline or shadow and there is no outlined variant.

Examples 🌈

Material spec states under the "Resting elevation and environment" heading that

The resting elevations on mobile are designed to provide visual cues, like shadows, to indicate when components are interactive. In contrast, resting elevations on desktop are shallower because other cues, like hover states, communicate when a component is interactive. For example, cards at 0dp elevation on desktop are outlined with a stroke.

and the second image in that section illustrates this.

The styles implemented in material-components-web could be an inspiration for this, but it basically boils down to applying a border correctly and making sure this does not impact spacing (e.g. in the Material docs example, the outline seamlessly merges into the media block at the top of the card).

Context 🔦

I am writing a desktop-only web app using a dense Material design and when there are lots of components on the screen, a flat outlined style is easier to look at and use.

@oliviertassinari oliviertassinari added component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request labels May 16, 2019
@oliviertassinari
Copy link
Member

@mjadczak Thank you for opening the issue. I wasn't aware the Material Specification was supporting it. I'm tempted to encourage this API:

<Paper variant="outlined">

It would invalidate the elevation prop.

@mbrookes
Copy link
Member

the outline seamlessly merges into the media block at the top of the card

That's going to be the tricky part. AFAIK, you can only do this with a background image (or color). The content is constrained to the content box. Instead the border would have to be an overlay that mirrors the Paper's size.

@oliviertassinari
Copy link
Member

@mbrookes What API do you prefer?

@mbrookes
Copy link
Member

@oliviertassinari The API you proposed looks fine, it's the implementation of it that may prove challenging.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 18, 2019

@mbrookes I would propose the following:

diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js
index 981db55f4..1db7e2040 100644
--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -23,6 +23,9 @@ export const styles = theme => {
     rounded: {
       borderRadius: theme.shape.borderRadius,
     },
+    outlined: {
+      border: `1px solid ${theme.palette.grey[300]}`,
+    },
     ...elevations,
   };
 };
@@ -34,6 +37,7 @@ const Paper = React.forwardRef(function Paper(props, ref) {
     component: Component = 'div',
     square = false,
     elevation = 1,
+    variant = 'elevation',
     ...other
   } = props;

@@ -44,9 +48,10 @@ const Paper = React.forwardRef(function Paper(props, ref) {

   const className = clsx(
     classes.root,
-    classes[`elevation${elevation}`],
     {
       [classes.rounded]: !square,
+      [classes.outlined]: variant === 'outlined',
+      [classes[`elevation${elevation}`]]: variant === 'elevation',
     },
     classNameProp,
   );
@@ -82,6 +87,7 @@ Paper.propTypes = {
    * If `true`, rounded corners are disabled.
    */
   square: PropTypes.bool,
+  variant: PropTypes.oneOf(['outlined', 'elevation']),
 };

 export default withStyles(styles, { name: 'MuiPaper' })(Paper);

@mjadczak What do you think?

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label May 18, 2019
@mjadczak
Copy link
Author

This seems reasonable to me, and would be ok for my needs, but what it doesn't address (I don't think), and what @mbrookes was talking about is this (from Material docs):
image
Note how the border is not around the entire card, but rather only around the bottom, with the media block being 1px wider in each direction to cover the border.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 18, 2019

@mjadczak It would work with:

box-shadow: inset 0px 0px 0px 1px red;

I'm not sure what would be the right API to support the border as well as the inset box shadow 🤔. Ideas?

@mjadczak
Copy link
Author

Ah, I didn't think of inset shadows. In fact, with this it means that you could enable this behaviour just by changing the theme (i.e. change the shadow for 0 elevation), right?

Is there any downside to this approach? If not, why do you think that both the border and inset shadow approach to this effect should be supported?

@oliviertassinari
Copy link
Member

@mjadczak Correct. I'm not aware of any downside. I'm having a hard time picturing a good API here.

@mjadczak
Copy link
Author

mjadczak commented May 21, 2019

What's wrong with the original API proposed, i.e.

<Paper variant="outlined">

?

Alternatively, this could be something set in the theme and apply to all elevation={0} paper?

@oliviertassinari
Copy link
Member

@mjadczak How would you expose an implementation that uses box-shadow and another one that uses border? I find the two approaches interesting. Should we expose both? Which one should we expose by default?

@mbrookes
Copy link
Member

It seems it should be outlined by default when elevation={0}:

image
https://material.io/design/components/cards.html#behavior

@oliviertassinari
Copy link
Member

@mbrookes Oh wow, that's great to know. How do we handle it 🤔?

@mbrookes
Copy link
Member

Something like this would do it:

--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import warning from 'warning';
 import withStyles from '../styles/withStyles';
+import { fade } from '../styles/colorManipulator';

 export const styles = theme => {
   const elevations = {};
@@ -24,6 +25,9 @@ export const styles = theme => {
       borderRadius: theme.shape.borderRadius,
     },
     ...elevations,
+    outlined: {
+      boxShadow: `inset 0px 0px 0px 1px ${fade(theme.palette.grey[300], 0.5)}`,
+    },
   };
 };

@@ -47,6 +51,7 @@ const Paper = React.forwardRef(function Paper(props, ref) {
     classes[`elevation${elevation}`],
     {
       [classes.rounded]: !square,
+      [classes.outlined]: elevation === 0,
     },
     classNameProp,
   );

But it's a breaking change. A variant prop that only applies when elevation=0 would be non-breaking, but increases the API surface.

@oliviertassinari
Copy link
Member

Maybe we could use the following API. The first value of the enum behind the default one.

variant?: 'elevation' | 'outlined';
outlined?: 'border' | 'boxShadow';

And in practice:

  • <Paper />
    Capture d’écran 2019-05-28 à 12 33 41
  • <Paper elevation={0} />
    Capture d’écran 2019-05-28 à 12 34 07
  • <Paper variant="outlined" />
    Capture d’écran 2019-05-28 à 12 35 22
  • <Paper variant="outlined" outlined="boxShadow" />
    Capture d’écran 2019-05-28 à 12 35 48

@mjadczak
Copy link
Author

I kind of feel like the boxShadow implementation should be the default, if it plays properly with having a media card header. I don't really see the advantage of having both implementations available—do you think there is one? I think it may just cause confusion.

@oliviertassinari
Copy link
Member

The advantage I see with the border approach is how popular it is, hence simpler to override. You have a border-color attribute and the option to compound the border with a box shadow at the same time. From my perspective, the image problem is not the most common use case. What should we optimize for?

@mjadczak
Copy link
Author

mjadczak commented Jun 5, 2019

Why do you say that the border approach is more popular? Just in terms of other material libraries using it? Even if this is the case, I kind of think that if you're going to the trouble of customising the outlined variant of your 0dp-elevated cards, you can handle doing that yourself through the classes prop (that's one of the great things about material-ui, after all!). I think having a property which does not change how the card looks (merely how the look is implemented) is not a good idea.

@oliviertassinari
Copy link
Member

@mjadczak I'm saying it's more popular because most of the UI components people are building/using follow this approach (we are more in the business of saving people time than implementing material design down to the last pixel). It's the least surprising option we have.

@joshwooding joshwooding added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Sep 30, 2019
@LorenzHenk
Copy link

What about using the outline css property?

diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js
index 981db55f4..1db7e2040 100644
--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -23,6 +23,9 @@ export const styles = theme => {
     rounded: {
       borderRadius: theme.shape.borderRadius,
     },
+    outlined: {
+      outline: `1px solid ${theme.palette.grey[300]}`,
+    },
     ...elevations,
   };
 };

@oliviertassinari oliviertassinari added component: card This is the name of the generic UI component, not the React module! and removed component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 6, 2019
@oliviertassinari oliviertassinari added new feature New feature or request priority: important This change can make a difference labels Oct 6, 2019
@oliviertassinari oliviertassinari added the component: Paper This is the name of the generic UI component, not the React module! label Nov 16, 2019
@oliviertassinari oliviertassinari removed the priority: important This change can make a difference label Dec 1, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 3, 2019

@LorenzHenk As far as I know, there is no radius support for the outline CSS property.

I spend more time looking at the problem, I think that we should go with the following approach:

diff --git a/packages/material-ui/src/Paper/Paper.d.ts b/packages/material-ui/src/Paper/Paper.d.ts
index 02de56d36..3d1a5fd31 100644
--- a/packages/material-ui/src/Paper/Paper.d.ts
+++ b/packages/material-ui/src/Paper/Paper.d.ts
@@ -6,11 +6,13 @@ export interface PaperProps
   component?: React.ElementType<React.HTMLAttributes<HTMLElement>>;
   elevation?: number;
   square?: boolean;
+  variant?: 'elevation' | 'outlined';
 }

 export type PaperClassKey =
   | 'root'
   | 'rounded'
+  | 'outlined'
   | 'elevation0'
   | 'elevation1'
   | 'elevation2'
diff --git a/packages/material-ui/src/Paper/Paper.js b/packages/material-ui/src/Paper/Paper.js
index 9b1a9ee36..0c042ba19 100644
--- a/packages/material-ui/src/Paper/Paper.js
+++ b/packages/material-ui/src/Paper/Paper.js
@@ -22,6 +22,9 @@ export const styles = theme => {
     rounded: {
       borderRadius: theme.shape.borderRadius,
     },
+    outlined: {
+      border: `1px solid ${theme.palette.divider}`,
+    },
     ...elevations,
   };
 };
@@ -33,6 +36,7 @@ const Paper = React.forwardRef(function Paper(props, ref) {
     component: Component = 'div',
     square = false,
     elevation = 1,
+    variant = 'elevation',
     ...other
   } = props;

@@ -46,9 +50,10 @@ const Paper = React.forwardRef(function Paper(props, ref) {
     <Component
       className={clsx(
         classes.root,
-        classes[`elevation${elevation}`],
         {
           [classes.rounded]: !square,
+          [classes[`elevation${elevation}`]]: variant === 'elevation',
+          [classes.outlined]: variant === 'outlined',
         },
         className,
       )}
@@ -86,6 +91,10 @@ Paper.propTypes = {
    * If `true`, rounded corners are disabled.
    */
   square: PropTypes.bool,
+  /**
+   * The variant to use.
+   */
+  variant: PropTypes.oneOf(['elevation', 'outlined']),
 };

 export default withStyles(styles, { name: 'MuiPaper' })(Paper);

While the box-shadow approach can look better, it's not always the case. Using border is safer, especially with light images. It's the approach used by Google Keep, Google Search rich snippets, and Vuetify.

Capture d’écran 2019-12-03 à 20 39 58
Capture d’écran 2019-12-03 à 20 40 12
Capture d’écran 2019-12-03 à 20 40 18
Capture d’écran 2019-12-03 à 20 40 25

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 3, 2019
@oliviertassinari oliviertassinari changed the title Support outlined Card / Paper [Paper] Support outlined Card / Paper Dec 3, 2019
@netochaves
Copy link
Contributor

Working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: card This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants