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

[MobileStepper] Add nextButton and backButton property #8001

Merged
merged 1 commit into from Sep 10, 2017
Merged

[MobileStepper] Add nextButton and backButton property #8001

merged 1 commit into from Sep 10, 2017

Conversation

wieseljonas
Copy link
Contributor

@wieseljonas wieseljonas commented Sep 1, 2017

I needed this to pass props to the MobileStepper component

Breaking change

+import KeyboardArrowLeft from 'material-ui-icons/KeyboardArrowLeft';
+import KeyboardArrowRight from 'material-ui-icons/KeyboardArrowRight';

     <MobileStepper
-        onBack={this.handleBack}
-        onNext={this.handleNext}
-        disableBack={this.state.activeStep === 0}
-        disableNext={this.state.activeStep === 5}
+        nextButton={
+          <Button dense onClick={this.handleNext} disabled={this.state.activeStep === 5}>
+            Next
+            <KeyboardArrowRight />
+          </Button>
+        }
+        backButton={
+          <Button dense onClick={this.handleBack} disabled={this.state.activeStep === 0}>
+            <KeyboardArrowLeft />
+            Back
+          </Button>
+        }
      />

@@ -121,6 +133,10 @@ MobileStepper.propTypes = {
/**
* Set the text that appears for the back button.
*/
backButtonProps: PropTypes.instanceOf(Object),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using PropTypes.object?

@@ -139,11 +155,15 @@ MobileStepper.propTypes = {
*/
disableNext: PropTypes.bool,
/**
* Passed into the onClick prop of the Back button.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong wording

@oliviertassinari oliviertassinari changed the title Add button props to MobileStepper [MobileStepper] Add button props Sep 1, 2017
@oliviertassinari oliviertassinari added component: stepper This is the name of the generic UI component, not the React module! v1 PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Sep 1, 2017
@wieseljonas
Copy link
Contributor Author

@oliviertassinari thanks for the feedback

@stunaz
Copy link
Contributor

stunaz commented Sep 1, 2017

Looking at this change and at the MobileSteppper Api itself, maybe instead of passing props (backButtonText, backButtonProps, disableBack, onBack ....) of button. Maybe it would be better if the MobileSteppper would accept 2 components (i.e. backButton and nextButton) as props: thinking about somehting like this ;

<MobileStepper
          type="text"
          steps={6}
          position="static"
          activeStep={this.state.activeStep}
          className={classes.mobileStepper}
          backComponent={<Button .../>}
          nextComponent={<Button .../>}
        />

@wieseljonas
Copy link
Contributor Author

@stunaz not a bad idea especially that now its not possible to remove the buttons at the first or last step

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 1, 2017

@stunaz I like this proposition. Following it we could make the MobileStepper API simpler. We could remove like 6 properties 👍.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 1, 2017

The following properties list

  • backButtonProps
  • backButtonText
  • disableBack
  • disableNext
  • nextButtonProps
  • nextButtonText
  • onBack
  • onNext

Could become:

  • backButton
  • nextButton

@wieseljonas
Copy link
Contributor Author

wieseljonas commented Sep 1, 2017

@oliviertassinari @stunaz Shall default buttons be implemented or like FormControlLabel should the prop is required?

@stunaz
Copy link
Contributor

stunaz commented Sep 1, 2017

I thinkrequired. As the user need to apply himself the disabled state

@wieseljonas
Copy link
Contributor Author

Should be good now

@oliviertassinari oliviertassinari added breaking change and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Sep 7, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning: Unknown props disableBack, disableNext on

tag. Remove these props from the element. For details, see

Some warnings in the docs

capture d ecran 2017-09-08 a 00 12 34

We might have to handle the size of the button. This doesn't look as in the specs.
(We need to apply the dense property on the button)


export interface MobileStepperProps extends PaperProps {
activeStep?: number;
backButtonProps: ButtonProps;
backButtonText?: React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove the old props.

@@ -119,9 +106,9 @@ MobileStepper.propTypes = {
*/
activeStep: PropTypes.number,
/**
* Set the text that appears for the back button.
* A button element. For instance, it can be be a `Button` or a `IconButton`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a back related comment.

@@ -131,25 +118,9 @@ MobileStepper.propTypes = {
*/
className: PropTypes.string,
/**
* Set to true to disable the back button.
* A button element. For instance, it can be be a `Button` or a `IconButton`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a next related comment.

@oliviertassinari oliviertassinari changed the title [MobileStepper] Add button props [MobileStepper] Add nextButton and backButton property Sep 10, 2017
@oliviertassinari oliviertassinari merged commit f042359 into mui:v1-beta Sep 10, 2017
@oliviertassinari
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: stepper This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants