[Tabs] Add scrollable behavior #6502

shawnmcknight · 2017-04-05T16:26:25Z

This PR adds the scrolling behavior requested in #4795 for both scrolling and arrows. Closes #1148.
Scrolling is treated as an opt-in prop and arrow button behavior can be customized to work automatically based on viewport size or forced on or off. Doc demos have been updated to show the various possibilities. Tests have been added and code coverage of module is at 100%.

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

shawnmcknight · 2017-04-05T17:54:18Z

The PR is failing on the vrtest run. I'm not really sure what to do about it since I'm not familiar with that tool and the docs seem to be wip.
I believe vrtest is comparing against a suite of baseline images. The live run of the test code looks very similar but it's probably not pixel identical because of the different css techniques involved in rendering the new structure.
I'm looking for some guidance and/or assistance on how to proceed with this. Thanks!

oliviertassinari · 2017-04-07T21:30:52Z

@shawnmcknight Hey, thanks for working on that feature! The UX is pretty cool 😍 . I have only noticed one issue so far, the demo do not render well on mobile (too large).

oliviertassinari

The overall implementation looks rock solid. Good work guys!

oliviertassinari · 2017-04-07T21:36:30Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonAuto.js

+
+const styleSheet = createStyleSheet('ScrollableTabsButtonAuto', (theme) => ({
+  root: {
+    width: 500,


I would use width: '100%' for all the example (to address the small width issue).

oliviertassinari · 2017-04-07T21:36:54Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonAuto.js

+            index={this.state.index}
+            onChange={this.handleChange}
+            scrollable
+            scrollButtons={'auto'}


scrollButtons="auto"

oliviertassinari · 2017-04-07T21:38:31Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonForce.js

+            textColor="accent"
+          >
+            <Tab
+              label="Item One"


I think that having the component on a single line would be better. At least our current rule is we stick 3 props then break it into different lines.

Maybe you should consider using https://github.com/prettier

@stunaz I looked at it a while back, and it was just too rigid / opinionated.

Isn't what we are looking for, something rigid and opinionated to focus on something else?

oliviertassinari · 2017-04-07T21:38:39Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonForce.js

+            index={this.state.index}
+            onChange={this.handleChange}
+            scrollable
+            scrollButtons={'on'}


scrollButtons="on"

oliviertassinari · 2017-04-07T21:39:57Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonPrevent.js

+            scrollButtons={'off'}
+          >
+            <Tab
+              icon={<PhoneIcon />}


Single line, the shorter the demos are the better.

oliviertassinari · 2017-04-07T22:05:58Z

src/Tabs/Tabs.js

+    const {
+      buttonClassName,
+      scrollable,
+      scrollButtons: scrollButtonsProp,


renaming the property to scrollButtonsProp seems point less.

oliviertassinari · 2017-04-07T22:06:10Z

src/Tabs/Tabs.js

      fullWidth,
      index,
      indicatorClassName,
      indicatorColor,
      onChange,
+      scrollable, // eslint-disable-line no-unused-vars
+      scrollButtons: scrollButtonsProp, // eslint-disable-line no-unused-vars


renaming the property to scrollButtonsProp seems point less.

oliviertassinari · 2017-04-07T22:06:57Z

src/Tabs/Tabs.js

+      showScrollButtons ? (
+        <TabScrollButton
+          className={buttonClassName}
+          direction={'right'}


direction="right"

oliviertassinari · 2017-04-07T22:07:09Z

src/Tabs/Tabs.js

+    conditionalElements.scrollButtonLeft = (
+      showScrollButtons ? (
+        <TabScrollButton
+          direction={'left'}


direction="left"

oliviertassinari · 2017-04-07T22:10:55Z

src/Tabs/Tabs.js


+  handleScrollbarSizeChange = ({ scrollbarHeight }) => {


Sounds like it's for supporting Windows 👍 .

shawnmcknight · 2017-04-09T13:51:43Z

@oliviertassinari I believe I've got everything you requested other than the dive() update. I tried for a while to get it working, but I was unable to get the shallow rendered withWidth() to let me find() or dive() or really anything at all. Any thoughts and/or advice on how to get this functional? I'll definitely update the tests to use it if I can get something working with it.

I'm also continuing to get the failure from the vrtest tool. I had attempted to get something added for this, but ran into problems with test:regressions. I had logged #6518 for that, but haven't heard anything so I'm stuck on that as well. If you could point me in a direction I can hopefully get this PR wrapped up.

Thanks!

I'm gonna have a look at it, thanks! I'm wondering what's going on with the vrtest.

oliviertassinari

Some last small things. It's close to be merged 😄 .

oliviertassinari · 2017-04-09T21:23:10Z

docs/src/pages/component-demos/tabs/ScrollableTabsButtonAuto.js

+// @flow weak
+/* eslint-disable react/no-multi-comp */
+
+import React, { Component, PropTypes } from 'react';


Could you rebase the branch on next we started the migration to use the prop-types packages.

oliviertassinari · 2017-04-09T21:31:44Z

src/svg-icons/keyboard-arrow-left.js

@@ -0,0 +1,15 @@
+/* eslint-disable */


I have updated the format, please rebase, here is an example of what it looks like now. I have just disabled some eslint rules instead of all of them, it's safer that way.

// @flow weak import React from 'react'; import pure from 'recompose/pure'; import SvgIcon from '../SvgIcon'; let CheckBox = (props) => ( <SvgIcon {...props}> <path d="M19 3H5c-1.11 0-2 .9-2 2v14c0 1.1.89 2 2 2h14c1.11 0 2-.9 2-2V5c0-1.1-.89-2-2-2zm-9 14l-5-5 1.41-1.41L10 14.17l7.59-7.59L19 8l-9 9z" /> </SvgIcon> ); CheckBox = pure(CheckBox); CheckBox.muiName = 'SvgIcon'; export default CheckBox;

oliviertassinari · 2017-04-09T21:37:05Z

Regarding the vrtest issue, I can always run the visual regression tests locally.

mbrookes · 2017-04-10T11:56:02Z

@shawnmcknight When you get a moment, please could you update https://github.com/callemall/material-ui/blob/next/docs/src/pages/getting-started/supported-components.md as part of this PR?

- Rebase/syntax requests - Support components docs - vrtest baseline image update

Visible now has a default propType of true and the not visible test requires an explicit declaration of false.

Awesome!

oliviertassinari

😍

oliviertassinari · 2017-04-11T11:25:30Z

@shawnmcknight Thanks a lot, I'm glad @STORIS is investing on the project 🙏 .

mbrookes · 2017-04-11T19:59:18Z

PS. Your website is down. 😱

ArsenyYankovsky · 2017-04-26T15:39:11Z

This actually broke tabs for me. Unfortunately I don't have time right now to make a reproducible example, but the fix is to check for this.tabs in updateScrollButtonState same as it's done in updatePositionStates. Thanks.

shawnmcknight · 2017-04-26T17:01:32Z

but the fix is to check for this.tabs in updateScrollButtonState same as it's done in updatePositionStates.

It's not immediately clear to me why that would be the case. Under what scenario are you ending up in updateScrollButtonState where there isn't a reference on this.tabs? updateScrollButtonState is invoked from three places: handleResize, handleTabsScroll, and updatePositionStates. In updatePositionStates, the invocation is already nested under the check for this.tabs. Both handleResize and handleTabsScroll should be events that are only fired once the component has rendered and the ref has been applied to this.tabs.

There shouldn't be any harm caused by adding a check for this.tabs to updateScrollButtonState but I'm just trying to understand the scenario that would cause a problem before making any change.

ArsenyYankovsky · 2017-04-26T17:21:42Z

@shawnmcknight
Well, I use it inside of a dialog and the error occurs at the moment dialog is being closed. It is not immediately reproducible because for some reason it only occurs if you scrolled a dialog with a vertical scroll.

shawnmcknight · 2017-04-26T20:23:47Z

I've been able to duplicate the issue, but not exactly the way described. I must put the tabs in a dialog, the tabs must be scrollable, and I must have scrolled the tabs. After that, dismissing the dialog will throw an error.

The issue seems to be due to events firing on a component that is in the process of being unmounted. The browser ends up firing events on something that has been dropped out of scope. We'd hide all of that nastiness by checking for this.tabs where you suggested, but I think the real problem is that we're rendering/scrolling the tab strip/firing scroll events when the dialog is dismissed.

I'll log an issue and get something comprehensive in place to improve performance while fixing the thrown error.

ArsenyYankovsky · 2017-04-26T20:43:13Z

Thanks. Also I don't think that function should be called if scrollButtons is set to 'off'.
Any ETA on the fix being in one of the new alpha versions?

oliviertassinari · 2017-04-26T21:05:48Z

I don't know if it's related but I have noticed one issue with how the throttle is handle. We need to cancel it during the unmount, here is an example.

shawnmcknight · 2017-04-26T21:09:41Z

@oliviertassinari There's a very good chance it's related, and might solve the null variable issue all on its own. I'd still like to improve the lifecycle handling to prevent unnecessary activity in the component, but the throttle cancel is probably a good start to addressing the issue.

@ArsenyYankovsky I'll get working on it asap, but I don't know how the PR will coincide with the timing of the next alpha release.

oliviertassinari · 2017-04-26T21:13:48Z

Impression of déjà vu 😄 . Regarding the releases of the next branch, we are doing them frequently, every week or so.

muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 5, 2017

oliviertassinari mentioned this pull request Apr 5, 2017

[test] Makes sure argos run even if diff fails #6512

Merged

3 tasks

shawnmcknight mentioned this pull request Apr 6, 2017

Error when running test:regressions #6518

Closed

muibot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: Needs Review labels Apr 7, 2017

mbrookes changed the title ~~[Tabs] Add scrollable behavior #4795~~ [Tabs] Add scrollable behavior Apr 7, 2017

oliviertassinari previously requested changes Apr 7, 2017

View reviewed changes

oliviertassinari added next and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Apr 7, 2017

muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 8, 2017

oliviertassinari removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 8, 2017

oliviertassinari mentioned this pull request Apr 8, 2017

[utils] Add test coverage for throttle #6544

Merged

3 tasks

muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 8, 2017

shawnmcknight added 3 commits April 9, 2017 07:20

[Tabs] Add scrollable behavior #4795

c54ffa0

Add lint overrides for unused props

3d519b5

Updated with changes requested

fce920f

muibot added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Apr 9, 2017

oliviertassinari previously requested changes Apr 9, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Apr 9, 2017

Added additional requests

db3efd1

- Rebase/syntax requests - Support components docs - vrtest baseline image update

muibot added the PR: Needs Review label Apr 10, 2017

Update TabScrollButton test due to default propType

4fcda0f

Visible now has a default propType of true and the not visible test requires an explicit declaration of false.

oliviertassinari approved these changes Apr 10, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted component: tabs This is the name of the generic UI component, not the React module! and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Apr 10, 2017

oliviertassinari merged commit ee5b8ad into mui:next Apr 11, 2017

shawnmcknight mentioned this pull request Apr 26, 2017

[Tabs] Scrollable Tabs throwing error when used inside dialog #6713

Closed

shawnmcknight mentioned this pull request Oct 6, 2017

support migration to material-ui@^0.19.0 STORIS/material-ui-scrollable-tabs#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tabs] Add scrollable behavior #6502

[Tabs] Add scrollable behavior #6502

shawnmcknight commented Apr 5, 2017

shawnmcknight commented Apr 5, 2017

oliviertassinari commented Apr 7, 2017 •

edited

Loading

oliviertassinari left a comment

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

stunaz Apr 7, 2017

mbrookes Apr 7, 2017

oliviertassinari Apr 8, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

oliviertassinari Apr 7, 2017

shawnmcknight commented Apr 9, 2017

oliviertassinari left a comment

oliviertassinari Apr 9, 2017

oliviertassinari Apr 9, 2017

oliviertassinari commented Apr 9, 2017

mbrookes commented Apr 10, 2017

oliviertassinari left a comment

oliviertassinari commented Apr 11, 2017

mbrookes commented Apr 11, 2017

ArsenyYankovsky commented Apr 26, 2017

shawnmcknight commented Apr 26, 2017

ArsenyYankovsky commented Apr 26, 2017 •

edited

Loading

shawnmcknight commented Apr 26, 2017

ArsenyYankovsky commented Apr 26, 2017

oliviertassinari commented Apr 26, 2017 •

edited

Loading

shawnmcknight commented Apr 26, 2017

oliviertassinari commented Apr 26, 2017

[Tabs] Add scrollable behavior #6502

[Tabs] Add scrollable behavior #6502

Conversation

shawnmcknight commented Apr 5, 2017

shawnmcknight commented Apr 5, 2017

oliviertassinari commented Apr 7, 2017 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnmcknight commented Apr 9, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 9, 2017

mbrookes commented Apr 10, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented Apr 11, 2017

mbrookes commented Apr 11, 2017

ArsenyYankovsky commented Apr 26, 2017

shawnmcknight commented Apr 26, 2017

ArsenyYankovsky commented Apr 26, 2017 • edited Loading

shawnmcknight commented Apr 26, 2017

ArsenyYankovsky commented Apr 26, 2017

oliviertassinari commented Apr 26, 2017 • edited Loading

shawnmcknight commented Apr 26, 2017

oliviertassinari commented Apr 26, 2017

oliviertassinari commented Apr 7, 2017 •

edited

Loading

ArsenyYankovsky commented Apr 26, 2017 •

edited

Loading

oliviertassinari commented Apr 26, 2017 •

edited

Loading