mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.42k stars 32.15k forks source link

[Tabs] Scroll buttons are not displayed if scrollButtons="auto" and on mobile #22598

Closed KyruCabading closed 4 years ago

KyruCabading commented 4 years ago

Current Behavior ๐Ÿ˜ฏ

When scrollButtons="auto" it never shows the scroll buttons even when tabs are not fully visible within Mobile View. The description of this prop says it should display the buttons whenever tabs are cut off.

From Tabs API

Determine behavior of scroll buttons when tabs are set to scroll:

  • auto will only present them when not all the items are visible. - desktop will only present them on medium and larger viewports. - on will always present them. - off will never present them.
Screen Shot 2020-09-14 at 12 19 01 PM

Expected Behavior ๐Ÿค”

Scroll buttons should display in mobile view if tabs are not fully visible.

Steps to Reproduce ๐Ÿ•น

You can see the behavior in this Codesandbox: https://codesandbox.io/s/material-demo-forked-gdrfp?file=/demo.js taken from https://github.com/mui-org/material-ui/issues/22592.

Context ๐Ÿ”ฆ

We have a subnavbar with scrollable tabs, we want the buttons to not appear (no left/right padding) in desktop since there's enough space to view all the tabs, but in mobile view, we want the scroll buttons to appear to signal to the user that there are additional items to see by scrolling right. We believe this is what the auto setting is supposed to achieve with scrollButtons prop.

Your Environment ๐ŸŒŽ

Tech Version
Material-UI ^4.9.5
KyruCabading commented 4 years ago

I think the solution is related to this line...

/* Styles applied to the `ScrollButtonComponent` component if `scrollButtons="auto"` or scrollButtons="desktop"`. */
  scrollButtonsDesktop: {
    [theme.breakpoints.down('xs')]: {
      display: 'none',
    },
  },

scrollButtons should never be hidden based on breakpoints if scrollButtons="auto"?

mnajdova commented 4 years ago

@oliviertassinari is this by design? I can see value of not showing them so that they won't occupy space, but it is not obvious that there are more items, which are not visible.

oliviertassinari commented 4 years ago

@mnajdova I couldn't remember, we did that 1 year ago, I have blamed the line to learn more about it. The logic comes from #15676, which links back to https://github.com/mui-org/material-ui/issues/15324#issuecomment-485774808. The current default behavior seems correct. It seems to be so we match the specification that never displays the buttons on mobile: https://material.io/components/tabs#anatomy.

On desktop, provide a visual indicator that more tabs are available off screen.

However, we can probably improve the DX, change the API. It seems that there are different dimensions:

mnajdova commented 4 years ago

The proposal on https://github.com/mui-org/material-ui/issues/15324#issuecomment-485774808 makes sense to me, I agree that probably we should have more options for the prop value. @KyruCabading does this make sense?

oliviertassinari commented 4 years ago

I'm not sure the proposal is up to date anymore. Would this API nail the problem?

  /**
   * Determine behavior of scroll buttons when tabs are set to scroll:
   *
   * - `auto` will only present them when not all the items are visible.
   * - `on` will always present them.
   * - `off` will never present them.
   *
   * Note that when present but disabled, the buttons are hidden with opacity: 0. They still reserve space.
   * @default 'auto'
   */
  scrollButtons: PropTypes.oneOf(['auto', 'off', 'on']),
  /**
   * If `true`, the scroll buttons are visible on mobile too.
   * @default false
   */
  disableHideScrollButtonsMobile: PropTypes.boolean,
oliviertassinari commented 4 years ago

@KyruCabading What do you think about this diff?

diff --git a/packages/material-ui/src/Tabs/Tabs.d.ts b/packages/material-ui/src/Tabs/Tabs.d.ts
index 0b07f9492c..1b9e14f3fe 100644
--- a/packages/material-ui/src/Tabs/Tabs.d.ts
+++ b/packages/material-ui/src/Tabs/Tabs.d.ts
@@ -63,6 +63,12 @@ export interface TabsTypeMap<P = {}, D extends React.ElementType = typeof Button
       /** Styles applied to the `TabIndicator` component. */
       indicator?: string;
     };
+    /**
+     * If `true`, the scroll buttons aren't forced hidden on mobile.
+     * Hidding scroll buttons with this prop takes precedence over `scrollButtons`.
+     * @default false
+     */
+    disableHideScrollButtonsMobile: boolean;
     /**
      * Determines the color of the indicator.
      * @default 'secondary'
@@ -89,12 +95,11 @@ export interface TabsTypeMap<P = {}, D extends React.ElementType = typeof Button
      * Determine behavior of scroll buttons when tabs are set to scroll:
      *
      * - `auto` will only present them when not all the items are visible.
-     * - `desktop` will only present them on medium and larger viewports.
      * - `on` will always present them.
      * - `off` will never present them.
      * @default 'auto'
      */
-    scrollButtons?: 'auto' | 'desktop' | 'on' | 'off';
+    scrollButtons?: 'auto' | 'on' | 'off';
     /**
      * If `true` the selected tab changes on focus. Otherwise it only
      * changes on activation.
diff --git a/packages/material-ui/src/Tabs/Tabs.js b/packages/material-ui/src/Tabs/Tabs.js
index 7495f28ed6..7caedf2eae 100644
--- a/packages/material-ui/src/Tabs/Tabs.js
+++ b/packages/material-ui/src/Tabs/Tabs.js
@@ -70,8 +70,8 @@ export const styles = (theme) => ({
   },
   /* Styles applied to the `ScrollButtonComponent` component. */
   scrollButtons: {},
-  /* Styles applied to the `ScrollButtonComponent` component if `scrollButtons="auto"` or scrollButtons="desktop"`. */
-  scrollButtonsDesktop: {
+  /* Styles applied to the `ScrollButtonComponent` component if `disableHideScrollButtonsMobile={true}`. */
+  scrollButtonsHideMobile: {
     [theme.breakpoints.down('xs')]: {
       display: 'none',
     },
@@ -90,6 +90,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) {
     classes,
     className,
     component: Component = 'div',
+    disableHideScrollButtonsMobile = false,
     indicatorColor = 'secondary',
     onChange,
     orientation = 'horizontal',
@@ -268,10 +269,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) {

     const scrollButtonsActive = displayScroll.start || displayScroll.end;
     const showScrollButtons =
-      scrollable &&
-      ((scrollButtons === 'auto' && scrollButtonsActive) ||
-        scrollButtons === 'desktop' ||
-        scrollButtons === 'on');
+      scrollable && ((scrollButtons === 'auto' && scrollButtonsActive) || scrollButtons === 'on');

     conditionalElements.scrollButtonStart = showScrollButtons ? (
       <ScrollButtonComponent
@@ -280,7 +278,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) {
         onClick={handleStartScrollClick}
         disabled={!displayScroll.start}
         className={clsx(classes.scrollButtons, {
-          [classes.scrollButtonsDesktop]: scrollButtons !== 'on',
+          [classes.scrollButtonsHideMobile]: !disableHideScrollButtonsMobile,
         })}
         {...TabScrollButtonProps}
       />
@@ -293,7 +291,7 @@ const Tabs = React.forwardRef(function Tabs(props, ref) {
         onClick={handleEndScrollClick}
         disabled={!displayScroll.end}
         className={clsx(classes.scrollButtons, {
-          [classes.scrollButtonsDesktop]: scrollButtons !== 'on',
+          [classes.scrollButtonsHideMobile]: !disableHideScrollButtonsMobile,
         })}
         {...TabScrollButtonProps}
       />

Do you want to work on the problem? :)

brorlarsnicklas commented 4 years ago

Can I work on a PR for this? :)

brorlarsnicklas commented 4 years ago

@oliviertassinari I followed the steps in the diff commented above, but I get an error when running yarn typescript (first picture) and if I try running yarn test I get the following (sec. picture).

I'm a bit new to this and not really sure what's missing, could you please help me figure out what is wrong?:) (Sorry for the wall of text in the first picture)

image

image

mbrookes commented 4 years ago

disableHideScrollButtonsMobile ๐Ÿคฎ

Couldn't we just add a mode to scrollButtons instead?

oliviertassinari commented 4 years ago

Couldn't we just add a mode to scrollButtons instead?

@mbrookes We would need to add onDesktop as a new variant, if we were to go down this road. But IMHO, it makes the logic harder to understand. It mixes two unrelated concerns into one prop.

mbrookes commented 4 years ago

I don't understand the variant requirement, but it isn't important - at least Gaurav had found a prop name that eliminate the doubled negative.

oliviertassinari commented 4 years ago

@mbrookes There are two concerns to consider regarding when the scroll buttons are displayed. It can depends on:

  1. The layout: does the the scroll container overflow?
  2. The screen width/touch target: do you want to display them on mobile?

Hence the proposal to have two different props. So far the tradeoff is to say: only display the scroll buttons if there is an overflow (auto), and don't display them on mobile (use visual tips that there is an overflow instead).

cbovis commented 3 years ago

For anyone that's interested, this can be solved using CSS. I'd agree that the default behaviour is optimal though since having an empty space on the left (when no arrow appears) looks weird. I put together a CSS solution which gets the best of both worlds. You'll want to adjust background color and spacing for your needs but here's what I'm using for reference:

scrollButtons: {
    width: 'initial',

    '&:first-child': {
      backgroundColor:
        theme.palette.type === 'dark'
          ? theme.palette.grey[800]
          : theme.palette.grey[50],
      paddingRight: theme.spacing(2),
      height: '100%',
      position: 'absolute',
      left: 0,
      opacity: 1,
      zIndex: 1,
    },

    '&.MuiTabs-scrollButtonsDesktop': {
      display: 'flex',

      '&.Mui-disabled:first-child': {
        visibility: 'hidden',
      },
    },
  }

This should be applied to the Tabs scrollButtons className.

yingying96 commented 3 years ago

May i know if the issue is resolved? I tried the CSS workarounds but it didn't work for me

alimorshedzadeh commented 2 years ago

I had similar issue in mobile mode with MUI v5.0.2 It will be working if you add allowScrollButtonsMobile to your component props

michalcode8 commented 3 months ago

make sure you also have variant="scrollable"

`<Tabs
          value={tabIndex}
          onChange={handleTabChange}
          variant="scrollable"
          allowScrollButtonsMobile
        >`