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

[Tooltip] Migrate to emotion #24571

Merged
merged 13 commits into from
Feb 26, 2021
Merged

Conversation

queengooborg
Copy link
Contributor

This PR migrates the Tooltip component to the new emotion format as a part of #24405.

@oliviertassinari oliviertassinari added the component: tooltip This is the name of the generic UI component, not the React module! label Jan 23, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 23, 2021

@material-ui/core: parsed: +0.26% , gzip: +0.21%
@material-ui/lab: parsed: +0.29% , gzip: +0.24%

Details of bundle changes

Generated by 🚫 dangerJS against 949065e

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

See the comment regarding the overridesResolver. Let me know if you are willing to take a look on this or if you need more help

const overridesResolver = (props, styles) => {
const { styleProps } = props;

return deepmerge(styles.popper || {}, {
Copy link
Member

Choose a reason for hiding this comment

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

As this component does not have a Root I would recommend creating two separate components TooltipPopper - you already have this one, and one wrapper for the children that will have the classes related to the tooltip. With that we will create two overridesResolver functions, once for each. At this moment I don't have better idea for solving this component. Cc @oliviertassinari

PS: The overrides resolver just spreads these styles on the root element, which in this case does not exists, that's why we need two separate overrides resolvers for this component.

Copy link
Member

@oliviertassinari oliviertassinari Jan 26, 2021

Choose a reason for hiding this comment

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

and one wrapper for the children

I don't follow the use case. From what I understand, we don't need to apply styles to the children. The component has no root.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2021
@oliviertassinari oliviertassinari self-assigned this Feb 23, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 25, 2021
@oliviertassinari oliviertassinari dismissed mnajdova’s stale review February 25, 2021 17:11

fix build and regressions

@oliviertassinari
Copy link
Member

@vinyldarkscratch Thanks!

@queengooborg queengooborg deleted the migrate/Tooltip branch February 26, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tooltip 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.

4 participants