Learus / react-material-ui-carousel

A Generic carousel UI component for React using Material UI.
MIT License
552 stars 220 forks source link

feat: make library more compatible with MUI v5 #143

Closed matyas-igor closed 2 years ago

matyas-igor commented 2 years ago

Motivation

Remove legacy dependency of @mui/styles with minimal code changes and maximal compatibility. Styling of sub-components with classes is still supported. Addresses https://github.com/Learus/react-material-ui-carousel/issues/130 and https://github.com/Learus/react-material-ui-carousel/issues/135.

List of changes

Learus commented 2 years ago

@matyas-igor Thanks! I appreciate the effort!

I like the idea of moving navButton props like fullHeightHover and alwaysInvisible into the navButtonProps prop, but in that case we need to remove them from the original props.

There is no point in having fullHeightHover in both the original props and the navButtonProps. Also, you forgot the to add the alwaysInvisible prop.

Is there a chance you could fix those issues?

matyas-igor commented 2 years ago

Hey @Learus, thanks a lot for your feedback!

Yes, I agree with your point. Now I'm thinking that it's probably better to remove passing fullHeightHover and alwaysVisible to NavButton since they're anyway available from the outside, so I'll probably just remove this part now. And anyway it could be added in another PR.

alwaysInvisible was not added on purpose, because rendering is called like {!alwaysInvisible && NavButton({ ... })} so it's gonna be false all the time :)

Another issue is that before className passed to NavButton was including many more styles:

    const buttonVisibilityClassValue = `${navButtonsAlwaysVisible ? classes.buttonVisible : classes.buttonHidden}`;
    const buttonCssClassValue = `${classes.button} ${buttonVisibilityClassValue} ${fullHeightHover ? classes.fullHeightHoverButton : ""} ${buttonsClass}`;
    ...
    NavButton({ className: buttonCssClassValue })

And now it's only buttonsClass:

    NavButton({ className: buttonsClass })

I think it's not super critical, but still may be considered as a breaking change... Should I maybe add passing the same styles as before via style prop to NavButton? What do you think?

Learus commented 2 years ago

I will have to check out how the new styling system works. When I am familiar with it and able to maintain it, I will merge this PR. Hopefully, that will be within the next few days.

Again, thanks for the effort! 😄