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

[SwipeableDrawer] Migrate SwipeArea to emotion #26059

Merged
merged 6 commits into from
Apr 30, 2021

Conversation

mnajdova
Copy link
Member

While trying to remove the dependency to withStyles, I noticed that this private component wasn't migrated to emotion.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 29, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 288607b

@oliviertassinari oliviertassinari added the component: SwipeableDrawer The React component. label Apr 29, 2021
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.

Oh well, good catch.

Comment on lines 20 to 21
name: 'PrivateSwipeArea',
slot: 'Root',
Copy link
Member

Choose a reason for hiding this comment

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

What the use case for exposing it?

Suggested change
name: 'PrivateSwipeArea',
slot: 'Root',

I think that we should remove this as well as the global class names. This component is private. It's what we do in: https://github.com/mui-org/material-ui/blob/77e4e292e3efa46640c02e92c91beb76f1099bbf/packages/material-ui/src/OutlinedInput/NotchedOutline.js#L7

Copy link
Member Author

@mnajdova mnajdova Apr 29, 2021

Choose a reason for hiding this comment

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

Fair enough see my next comment

Copy link
Member

Choose a reason for hiding this comment

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

If we need to customize it, I think that SwipeableDrawer could create a new slot for it and apply it as a class name. We have used this approach in the past.

For the pickers, we went in a different direction with @dmtrKovalenko. We made the classes API of the internal component's public to ease the customization. It's an oxymoron but I'm not sure we have better alternatives.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why I added the utility classes is because the SwipeableDrawer is not styled at all. That’s why I thought I could add this as root and make it available for customization. I would even add the overrides resolver here, as there is no other point for injecting customization.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot that SwipeableDrawer had no styles. If we want to make a customization slot for SwipeArea public in the future, I think that the approach used here will make sense.

@mnajdova
Copy link
Member Author

Oh well, good catch.

There is another one for tomorrow 😂

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Is this really the simplest possible approach for creating a styled component? Wasn't one of the goals to be able to leverage props in styles. And now we still have all these classes, we even need more boilerplate. And all that for a couple of lines of styles.

And let's not think about customizing this component yet. It's not an issue at the moment

@mnajdova
Copy link
Member Author

Is this really the simplest possible approach for creating a styled component? Wasn't one of the goals to be able to leverage props in styles. And now we still have all these classes, we even need more boilerplate. And all that for a couple of lines of styles.

And let's not think about customizing this component yet. It's not an issue at the moment

Simplified, done just the minimal changes needed.

@mnajdova
Copy link
Member Author

@oliviertassinari @eps1lon needed to add the PrivateSwipeArea-root class as otherwise tests were failing https://app.circleci.com/pipelines/github/mui-org/material-ui/41433/workflows/722928ac-f470-41f9-9174-fb6aefd7b915/jobs/247257. For simplicity I just hardcoded it in the implementaiton.

@mnajdova mnajdova merged commit fa432cc into mui:next Apr 30, 2021
@oliviertassinari oliviertassinari added the component: drawer This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: drawer This is the name of the generic UI component, not the React module! component: SwipeableDrawer The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants