[Stepper] Mobile version #7043

alexhayes · 2017-06-03T13:58:36Z

As discussed in issue #7033 this PR implements the three variations of mobile stepper outlined in the material guidelines.

Closes #7033.

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).

Screenshots taken from the included docs;

Dots

Text

Progress

oliviertassinari

Yeah, thanks a lot for working on that! 🙏 💯
That's a first quite review. I need to spend more time on it

oliviertassinari · 2017-06-03T18:58:04Z

docs/src/pages/getting-started/supported-components.md

@@ -96,6 +96,7 @@ to discuss the approach before submitting a PR.
 - [Steppers](https://www.google.com/design/spec/components/steppers.html)
  - [Horizontal](https://www.google.com/design/spec/components/steppers.html#steppers-types-of-steppers)
  - [Vertical](https://www.google.com/design/spec/components/steppers.html#steppers-types-of-steppers)
+  - [MobileStepper](https://www.google.com/design/spec/components/mobile-stepper.html)


I guess we could mark it done?

When it's merged :)

oliviertassinari · 2017-06-03T18:58:49Z

src/MobileStepper/MobileStepper.js

+import { LinearProgress } from '../Progress';
+
+export const styleSheet = createStyleSheet('MuiMobileStepper', theme => ({
+  mobileStepper: {


I would rather call it root to follow the other components convention.

Will do - just an FYI tho, I was basing my work on the AppBar component which uses appBar not root.

oliviertassinari · 2017-06-03T18:59:49Z

src/MobileStepper/MobileStepper.js

+    left: 0,
+    zIndex: theme.zIndex.mobileStepper,
+    backgroundColor: theme.palette.background.paper,
+    padding: '6px',


The following work just fine. Also, I'm wondering if we shouldn't use the standard 8 px spacing with theme.spacing.unit.

padding: 6,

oliviertassinari · 2017-06-03T19:00:10Z

src/MobileStepper/MobileStepper.js

+  dot: {
+    backgroundColor: theme.palette.action.disabled,
+    borderRadius: '50%',
+    width: '10px',


Same we can use a number here.

oliviertassinari · 2017-06-03T19:00:56Z

src/MobileStepper/MobileStepper.js

+function MobileStepper(props) {
+  const {
+    activeStep,
+    buttonClassName: buttonClassNameProp,


I don't think that we need that property as already accessible with classes.button.

Same though for the other properties.

oliviertassinari · 2017-06-03T19:01:30Z

src/MobileStepper/MobileStepper.js

+  return (
+    <Paper square elevation={0} className={className} {...other}>
+      <Button className={buttonClassName} onClick={onBack} disabled={disableBack}>
+        <KeyboardArrowLeft /> Back


What about i118n? Maybe some label property?

oliviertassinari · 2017-06-03T19:02:30Z

src/MobileStepper/MobileStepper.js

+          {Array.from(Array(steps)).map((_, step) => {
+            const dotClassName = classNames(
+              {
+                [classes.dot]: true,


If it's always true, you can pass is as an argument, that's simpler.

oliviertassinari · 2017-06-03T19:04:18Z

src/styles/zIndex.js

@@ -12,4 +12,5 @@ export default {
  popover: 2100,
  snackbar: 2900,
  tooltip: 3000,
+  mobileStepper: 1100,


We sort theme by index value, not the right position.

I think you are referring to this conversation but this is not related - I've done this because I envisaged this being sticky & fixed to the bottom of the mobile device.

oliviertassinari · 2017-06-03T19:08:10Z

src/MobileStepper/MobileStepper.js

+    position: 'fixed',
+    bottom: 0,
+    left: 0,
+    zIndex: theme.zIndex.mobileStepper,


Hum, I would rather not have any positioning property in our low-level components. One might position his element using flexbox only and not fixed positioning. (that's what we do, that would conflict). Maybe we could add a fix position option? (Would also help with the demos!) I'm tempted to say that de default option should not be fixed.

I've had a bit of a think about this and I think perhaps by default I'll remove the fixed positioning, however I think a very common use case would be for it to be fixed either at the top or bottom so I'll add flags to trigger this.

I've added a prop position in 65c70e3 which can be either bottom or top. By default MobileStepper is now not fixed.

…eight

alexhayes · 2017-06-04T09:59:11Z

@oliviertassinari FYI I think I've addressed your concerns - I'll await your input when you're ready.

oliviertassinari · 2017-06-05T09:59:50Z

@alexhayes Thanks! I took the liberty to make some changes. Let me know what you think.

alexhayes · 2017-06-05T10:29:54Z

@oliviertassinari sorry yes I saw that but didn't have time to comment today before you merged. I'll comment now anyway and I'll create a separate PR if you think it's necessary.

alexhayes

@oliviertassinari let us know if you'd like me to submit any of these as a PR.

alexhayes · 2017-06-05T10:34:24Z

docs/src/pages/component-api/MobileStepper/MobileStepper.md

-| dotClassName | string |  | Specify an extra class to be put on each dot element |
-| dotsClassName | string |  | Specify an extra class to be put the container that holds the dots |
-| progressClassname | string |  | Specify an extra class to be put the container that holds the <LinearProgress /> component. |
+| activeStep | number | 0 | Set the active step (zero based index). This will enable `Step` control helpers. |


I'm not sure the last sentence, This will enable Step control helpers., makes much sense.

activeStep doesn't enable something, it more sets something (in this case, the active property on a dot or the position on the progress component).

alexhayes · 2017-06-05T10:39:53Z

docs/src/pages/component-api/MobileStepper/MobileStepper.md

+| disableBack | bool | false | Set to true to disable the back button. |
+| disableNext | bool | false | Set to true to disable the next button. |
+| nextButtonText | node | 'Next' | Set the text that appears for the next button. |
+| <span style="color: #31a148">onBack *</span> | function |  | Passed into the onTouchTap prop of the Back button. |


It's actually passed into the onClick of the button. I did this because I couldn't find any mention of onTouchTap in either the docs or the Button or BaseButton components in the next branch.

I think either the docs need to state onClick or the code needs to be changed to use onTouchTap if that is still the way to do it.

Right, we need to use onClick.

alexhayes · 2017-06-05T10:40:05Z

docs/src/pages/component-api/MobileStepper/MobileStepper.md

+| disableNext | bool | false | Set to true to disable the next button. |
+| nextButtonText | node | 'Next' | Set the text that appears for the next button. |
+| <span style="color: #31a148">onBack *</span> | function |  | Passed into the onTouchTap prop of the Back button. |
+| <span style="color: #31a148">onNext *</span> | function |  | Passed into the onTouchTap prop of the Next button. |


alexhayes · 2017-06-05T10:50:39Z

src/MobileStepper/MobileStepper.js

@@ -17,22 +17,24 @@ export const styleSheet = createStyleSheet('MuiMobileStepper', theme => ({
    flexDirection: 'row',
    justifyContent: 'space-between',
    alignItems: 'center',
-    width: '100%',
-    backgroundColor: theme.palette.background.paper,
-    padding: theme.spacing.unit,


The removal of this means the padding on hover now looks out;

alexhayes · 2017-06-05T10:55:38Z

docs/src/pages/component-api/MobileStepper/MobileStepper.md

+| <span style="color: #31a148">onNext *</span> | function |  | Passed into the onTouchTap prop of the Next button. |
+| position | enum:&nbsp;'bottom'<br>&nbsp;'top'<br>&nbsp;'static'<br> | 'bottom' | Set the text that appears for the next button. |
+| <span style="color: #31a148">steps *</span> | number |  | The total steps. |
+| type | enum:&nbsp;'text'<br>&nbsp;'dots'<br>&nbsp;'progress'<br> | 'dots' | The type of mobile stepper to use. |


I'm not really sure what is gained here by using the   apart from making it harder to read (when viewed raw).

I tried out a simple mixed with the <br> and it worked fine.

This is automatically generated with yarn build:docs.

alexhayes · 2017-06-05T10:56:16Z

docs/src/pages/component-demos/stepper/DotsMobileStepper.js

+
+const styleSheet = createStyleSheet('DotsMobileStepper', {
+  root: {
+    maxWidth: 400,


alexhayes · 2017-06-05T10:59:48Z

docs/src/pages/component-demos/stepper/TextMobileStepper.js

    });
  };

  render() {
    const classes = this.props.classes;
    return (
      <div className={classes.root}>
-        <Paper square elevation={0} className={classes.textualDescription}>
-          Step {this.state.activeStep + 1} of 6
+        <Paper square elevation={0} className={classes.header}>


This no longer lines up with the < BACK button.

While it's an issue with the docs and not an issue with the material-ui codebase I think it does look a little off.

I'm not sure how it should line up.

alexhayes · 2017-06-05T11:02:05Z

docs/src/pages/component-demos/stepper/stepper.md

+
+## Mobile Stepper
+
+The [mobile steps](https://material.io/guidelines/components/steppers.html#steppers-types-of-steps) implements a compact stepper suitable for a mobile device.


I think this reads a little oddly. Perhaps;

This component implements a compact stepper suitable for a mobile device. See mobile steps for it's inspiration.

oliviertassinari · 2017-06-05T12:26:39Z

@alexhayes Thanks for the feedback. Would you mind submitting a PR to address those? 😄 .

alexhayes · 2017-06-07T20:00:18Z

@oliviertassinari Yep I should have some more time this week to submit that.

oliviertassinari · 2017-06-07T20:03:44Z

Awesome!

oliviertassinari · 2017-06-12T11:36:44Z

@alexhayes Any luck? I will do it otherwise.

alexhayes · 2017-06-12T11:54:21Z

Sorry, I've had a busy week and haven't had a chance yet. I should get some time mid this week.

* [MobileStepper] Ensure correct spacing around buttons in MobileStepper. * [MobileStepper] Documentation changes as per discussion on #7043.

[Stepper] Mobile version - fixes mui#7033

d5e4b19

alexhayes force-pushed the mobile-stepper branch from f7e1917 to d5e4b19 Compare June 3, 2017 14:18

alexhayes changed the title ~~WIP - Fixes #7033 - [Stepper] Mobile version~~ [Stepper] Mobile version Jun 3, 2017

oliviertassinari added the next label Jun 3, 2017

oliviertassinari reviewed Jun 3, 2017

View reviewed changes

oliviertassinari added the component: stepper This is the name of the generic UI component, not the React module! label Jun 3, 2017

alexhayes added 7 commits June 4, 2017 10:32

Change classes.mobileStepper for classes.root

ad89ff4

Use theme.spacing.unit rather than hard coded padding and dot width/h…

0aa9036

…eight

Optimise use of classNames for dotClassName

ec9275a

Fixed classes.root tests

5fbada3

Added props backButtonText and nextButtonText

c9b9011

Added position prop making MobileStepper not fixed by default.

65c70e3

Fixed linting issues

ee8c615

oliviertassinari mentioned this pull request Jun 4, 2017

[AppBar] Add a position property #7049

Merged

3 tasks

[MobileStepper] Second iteration

85150f1

oliviertassinari force-pushed the mobile-stepper branch from d57fd2c to 85150f1 Compare June 4, 2017 11:36

oliviertassinari added the PR: Review Accepted label Jun 4, 2017

oliviertassinari merged commit de4c31b into mui:next Jun 5, 2017

alexhayes commented Jun 5, 2017

View reviewed changes

alexhayes added a commit to alexhayes/material-ui that referenced this pull request Jun 19, 2017

[MobileStepper] Documentation changes as per discussion on mui#7043.

253c309

alexhayes mentioned this pull request Jun 19, 2017

[MobileStepper] Improvements #7179

Merged

3 tasks

oliviertassinari pushed a commit that referenced this pull request Jun 19, 2017

[MobileStepper] Improvements (#7179)

3408eec

* [MobileStepper] Ensure correct spacing around buttons in MobileStepper. * [MobileStepper] Documentation changes as per discussion on #7043.


		## Mobile Stepper

		The [mobile steps](https://material.io/guidelines/components/steppers.html#steppers-types-of-steps) implements a compact stepper suitable for a mobile device.

[Stepper] Mobile version #7043

[Stepper] Mobile version #7043

Conversation

alexhayes commented Jun 3, 2017 • edited by oliviertassinari

Dots

Text

Progress

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jun 3, 2017 • edited

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

Choose a reason for hiding this comment

alexhayes commented Jun 4, 2017

oliviertassinari commented Jun 5, 2017

alexhayes commented Jun 5, 2017

alexhayes 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

oliviertassinari Jun 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 5, 2017

alexhayes commented Jun 7, 2017

oliviertassinari commented Jun 7, 2017

oliviertassinari commented Jun 12, 2017

alexhayes commented Jun 12, 2017

alexhayes commented Jun 3, 2017 •

edited by oliviertassinari

oliviertassinari Jun 3, 2017 •

edited

oliviertassinari Jun 5, 2017 •

edited