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

Implement DrawerMenuItem #15107

Open
2 tasks done
raymondsze opened this issue Mar 29, 2019 · 7 comments
Open
2 tasks done

Implement DrawerMenuItem #15107

raymondsze opened this issue Mar 29, 2019 · 7 comments
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request

Comments

@raymondsze
Copy link
Contributor

raymondsze commented Mar 29, 2019

Drawer MenuItem: https://material-components.github.io/material-components-web-catalog/#/component/drawer
Demo:
https://material-components.github.io/material-components-web-catalog/#/component/drawer

  1. The Drawer MenuItem could capture the keyboard event.
  2. The Drawer MenuItem show the primary color when selected.
  3. The Drawer MenuItem have the rounded shape.
  4. The Drawer MenuItem Text is highlighted with darker primary color.

Shape: https://material.io/design/shape/about-shape.html#shape-customization-tool

  1. Shape could apply to small, medium and large components.
  2. This could be easily done by providing HOC or something else to change the root classes radius, but this is not easy for TextField, especially Outlined TextField and Filled TextFIeld.
  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Benchmark

@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Apr 11, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 11, 2019

I agree, I think that it's something we should implement and use in our documentation website.

Google has customized the style, it would make a great customization example:

Capture d’écran 2019-04-11 à 15 20 14


Capture d’écran 2019-04-11 à 15 20 01

@raymondsze
Copy link
Contributor Author

Do you think it should be an enhancenent of MenuItem/ListItem or a seperate implementation?

@eps1lon

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title Is there any plan to implement the DrawerMenuItem and ShapedComponent ? Implement DrawerMenuItem Apr 12, 2019
@oliviertassinari
Copy link
Member

We have a couple of different efforts and issue on the List component, I think that it would be great to align them, cc @mbrookes:

I have made a quick POC to get an idea on the options we have:

Capture d’écran 2019-04-14 à 19 45 59

Git diff
--- a/docs/src/pages/demos/lists/SelectedListItem.js
+++ b/docs/src/pages/demos/lists/SelectedListItem.js
@@ -26,7 +26,7 @@ function SelectedListItem() {

   return (
     <div className={classes.root}>
-      <List component="nav">
+      <List variant="nav" component="nav">
         <ListItem
           button
           selected={selectedIndex === 0}
diff --git a/packages/material-ui/src/List/List.js b/packages/material-ui/src/List/List.js
index 0039c4c95..b85add49b 100644
--- a/packages/material-ui/src/List/List.js
+++ b/packages/material-ui/src/List/List.js
@@ -32,12 +32,13 @@ const List = React.forwardRef(function List(props, ref) {
     className,
     component: Component,
     dense,
+    variant,
     disablePadding,
     subheader,
     ...other
   } = props;

-  const context = React.useMemo(() => ({ dense }), [dense]);
+  const context = React.useMemo(() => ({ dense, variant }), [dense, variant]);

   return (
     <ListContext.Provider value={context}>
@@ -94,10 +95,12 @@ List.propTypes = {
    * The content of the subheader, normally `ListSubheader`.
    */
   subheader: PropTypes.node,
+  variant: PropTypes.oneOf(['nav', 'ul']),
 };

 List.defaultProps = {
   component: 'ul',
+  variant: 'ul',
   dense: false,
   disablePadding: false,
 };
diff --git a/packages/material-ui/src/ListItem/ListItem.js b/packages/material-ui/src/ListItem/ListItem.js
index daa06ec91..2be1632d3 100644
--- a/packages/material-ui/src/ListItem/ListItem.js
+++ b/packages/material-ui/src/ListItem/ListItem.js
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import { chainPropTypes } from '@material-ui/utils';
 import withStyles from '../styles/withStyles';
+import { fade } from '../styles/colorManipulator';
 import ButtonBase from '../ButtonBase';
 import { isMuiElement } from '../utils/reactHelpers';
 import ListContext from '../List/ListContext';
@@ -15,7 +16,7 @@ export const styles = theme => ({
     alignItems: 'center',
     position: 'relative',
     textDecoration: 'none',
-    width: '100%',
+    //width: '100%',
     boxSizing: 'border-box',
     textAlign: 'left',
     paddingTop: 8,
@@ -24,6 +25,15 @@ export const styles = theme => ({
       backgroundColor: theme.palette.action.selected,
     },
   },
+  variantNav: {
+    margin: 8,
+    overflow: 'hidden',
+    '&$selected, &$selected:hover': {
+      borderRadius: 4,
+      color: theme.palette.primary.dark,
+      backgroundColor: fade(theme.palette.primary.main, theme.palette.action.hoverOpacity),
+    },
+  },
   /* Styles applied to the `container` element if `children` includes `ListItemSecondaryAction`. */
   container: {
     position: 'relative',
@@ -106,12 +116,15 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
   const childContext = {
     dense: dense || context.dense || false,
     alignItems,
+    selected,
   };

   const children = React.Children.toArray(childrenProp);
   const hasSecondaryAction =
     children.length && isMuiElement(children[children.length - 1], ['ListItemSecondaryAction']);

+  console.log('context', context)
+
   const componentProps = {
     className: clsx(
       classes.root,
@@ -120,6 +133,7 @@ const ListItem = React.forwardRef(function ListItem(props, ref) {
         [classes.gutters]: !disableGutters,
         [classes.divider]: divider,
         [classes.disabled]: disabled,
+        [classes.variantNav]: context.variant === 'nav',
         [classes.button]: button,
         [classes.alignItemsFlexStart]: alignItems === 'flex-start',
         [classes.secondaryAction]: hasSecondaryAction,
diff --git a/packages/material-ui/src/ListItemIcon/ListItemIcon.js b/packages/material-ui/src/ListItemIcon/ListItemIcon.js
index 89e61d727..41eed6824 100644
--- a/packages/material-ui/src/ListItemIcon/ListItemIcon.js
+++ b/packages/material-ui/src/ListItemIcon/ListItemIcon.js
@@ -2,6 +2,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
+import ListContext from '../List/ListContext';

 export const styles = theme => ({
   /* Styles applied to the root element. */
@@ -11,6 +12,9 @@ export const styles = theme => ({
     flexShrink: 0,
     display: 'inline-flex',
   },
+  selected: {
+    color: 'inherit',
+  },
 });

 /**
@@ -18,8 +22,15 @@ export const styles = theme => ({
  */
 const ListItemIcon = React.forwardRef(function ListItemIcon(props, ref) {
   const { classes, className, ...other } = props;
+  const context = React.useContext(ListContext);

-  return <div className={clsx(classes.root, className)} ref={ref} {...other} />;
+  return (
+    <div
+      className={clsx(classes.root, { [classes.selected]: context.selected }, className)}
+      ref={ref}
+      {...other}
+    />
+  );
 });

 ListItemIcon.propTypes = {

What about:

We follow the MenuItem pattern, we introduce a new DrawerListItem component, it's a drop in replacement for the ListItem component. It has a custom handling of the selected property as shown in the previous git diff. If somebody wants its drawer list to be keyboard navigable, it replaces the List component with the MenuList component?

@mbrookes mbrookes added umbrella For grouping multiple issues to provide a holistic view and removed umbrella For grouping multiple issues to provide a holistic view labels Apr 16, 2019
@mbrookes
Copy link
Member

Alternative strategy:

Standardise on a single List and ListItem component. Add variants for Drawer and Menu. Add a disableAccessibility (😜) prop to disable Menu style keyboard navigation.

We would need to wait for 4.1 for deprecation of MenuList and MenuListItem, however it could be implemented now, with MenuList and MenuListItem becoming a thin wrapper that sets the List variant, with deprecation added later. (Or, the wrappers could become permanent for API stability...)

@raymondsze
Copy link
Contributor Author

variant: "nav" may not be suitable?
The MWC has this kind of style for Menu of SelectField as well. For that case, the parent component is ul but with primary style. Although Material-UI does not necessary to follow MWC.

Screenshot 2019-06-27 at 12 24 37 PM

@oliviertassinari oliviertassinari added the new feature New feature or request label Oct 4, 2020
@oliviertassinari
Copy link
Member

We explore this problem further in #22780 for the requirements of the documentation. This will be a great starting point to work on this issue. There is a decision to take between bundling the style inside ListItem (1), creating a new component that extends ListItem (2), or creating a new component without ListItem (3).

With hindsight, I think that we should have used option 3 for the Menu: it helps customization (fewer layers of abstraction, style often different) and performance (fewer components, especially relevant with lists).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants