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

[Stepper] Remove Paper and built-in padding #22564

Merged
merged 12 commits into from
Sep 14, 2020

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Sep 11, 2020

Breaking changes

  • [Stepper] The root component (Paper) was replaced with a <div>. Stepper no longer has elevation, nor inherits Paper's props.
-<Stepper elevation={2}>
-  <Step>
-    <StepLabel>Hello world</StepLabel>
-  </Step>
-</Stepper>
+<Paper square elevation={2}>
+  <Stepper>
+    <Step>
+      <StepLabel>Hello world</StepLabel>
+    </Step>
+  </Stepper>
+<Paper>
  • [Stepper] Remove the built-in 24px padding.
-<Stepper>
-  <Step>
-    <StepLabel>Hello world</StepLabel>
-  </Step>
-</Stepper>
+<Stepper style={{ padding: 24 }}>
+  <Step>
+    <StepLabel>Hello world</StepLabel>
+  </Step>
+</Stepper>

Closes #18423

@mbrookes mbrookes added component: stepper This is the name of the generic UI component, not the React module! breaking change labels Sep 11, 2020
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.

I think that with the change, we can use the default outlined container for the demos, instead of the grey background.

docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 11, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 09a3cf2

mbrookes and others added 3 commits September 11, 2020 21:57
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
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.

The mobile stepper still has a white background, it's easier to spot with:

docs/src/pages/components/steppers/steppers.md Outdated Show resolved Hide resolved
docs/src/pages/components/steppers/steppers.md Outdated Show resolved Hide resolved
docs/src/pages/components/steppers/steppers.md Outdated Show resolved Hide resolved
docs/src/pages/components/steppers/steppers.md Outdated Show resolved Hide resolved
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mbrookes
Copy link
Member Author

mbrookes commented Sep 11, 2020

@oliviertassinari I've tweaked the Button layout and variant for the first demo to better match the original component:

Spec:
https://material.io/archive/guidelines/components/steppers.html#steppers-types-of-steppers
image

Before:
image

After:
image

Once it's good, I'll do the rest of the demos.

@oliviertassinari
Copy link
Member

@mbrookes Looks better 👍

@mbrookes
Copy link
Member Author

Option 2 – add a white background so you can distinguish the component boundaries from the demo container padding, add a divider:

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2020

@mbrookes I think that option 1 is better. It will make the source of the demo simpler. Also, I don't think that the stepper should have any padding.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2020

I had to override the padding in the store:

Capture d’écran 2020-09-12 à 00 21 26

And if you are ShutterStock (yes the store is inspired by shutterstock.com & themes.getbootstrap.com), you would need too:

Capture d’écran 2020-09-12 à 00 22 35


What about we removing this padding?

@mbrookes
Copy link
Member Author

I don't think that the stepper should have any padding.

I agree. I just didn't want to be too radical!

Mk 3:

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 12, 2020

I agree. I just didn't want to be too radical!

Oh right, one breaking change at the time, removal of padding could be the very next one 👼.

Mk 3:

A couple of thoughts:

  1. Having the buttons closer to each other makes it easier to use the playground. Otherwise, you need the mouse to make more movement to go back.
  2. Maybe we should have one static demos of the Stepper at the top of the page with an inline preview that showcase the composition API. The demos are quite long and can be hard to navigate.
  3. For the customized stepper, I think that we can drop the buttons, make is static, and have step 2 as active: https://deploy-preview-22564--material-ui.netlify.app/components/steppers/#customized-stepper. It would cover all the use cases and simplify the demo.
  4. Do we need the labels like "This is the bit I really care about!", What if we already displayed the active step? e.g. "Step 2" => simpler demos.
  5. Removing the built-in padding of the Stepper would allow to simply the demos, as would allow to remove the padding we add on the controls.

@mbrookes
Copy link
Member Author

mbrookes commented Sep 12, 2020

We had a power cut this afternoon, but have been working offline on simplifying the demos. It seems I've already hit on several of the things you suggested. Take a look.

Having the buttons closer to each other makes it easier to use the playground. Otherwise, you need the mouse to make more movement to go back.

That's down to the demo not having a container. You can't have it both ways. 😄
You don't really need to use the back button to navigate the demos.

Maybe we should have one static demos of the Stepper at the top of the page with an inline preview that showcase the composition API.

I think we need to show a functioning Stepper first.

The demos are quite long and can be hard to navigate.

Most of the demos are now simplified.

For the customized stepper, I think that we can drop the buttons

Already done.

What if we already displayed the active step? "Step 2" => simpler demos

Already simpler, but I'll take another look.

Removing the built-in padding of the Stepper

Already done in "Mk 3".

would allow to remove the padding we add on the controls.

Already done.

@oliviertassinari oliviertassinari changed the title [Stepper] Remove Paper [Stepper] Remove Paper and built-in padding Sep 13, 2020
@mbrookes mbrookes merged commit f7d4af9 into mui:next Sep 14, 2020
@mbrookes mbrookes deleted the stepper-remove-paper branch September 14, 2020 19:47
@oliviertassinari oliviertassinari added this to the v5 milestone Sep 15, 2020
@oliviertassinari oliviertassinari mentioned this pull request Sep 15, 2020
42 tasks
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.

[Stepper] Remove Paper component
3 participants