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] Fix the property forwarding priority #13667

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 21, 2018

The change is summarised by this test case:

    it('should respect the properties priority', () => {
      const wrapper = shallow(
        <Tooltip className="foo" {...defaultProps}>
          <h1 className="bar">H1</h1>
        </Tooltip>,
      );
      const className = wrapper.find('h1').props().className;

      // Before : className === 'foo'
      // After : className === 'bar'
    });

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: tooltip This is the name of the generic UI component, not the React module! labels Nov 21, 2018
@@ -103,7 +103,6 @@ class SpeedDialAction extends React.Component {
placement={tooltipPlacement}
onClose={this.handleTooltipClose}
onOpen={this.handleTooltipOpen}
className={classes.root}
Copy link
Member

Choose a reason for hiding this comment

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

We do the same thing for Button though. Sure that is a little bit different since Button has actual styles in root. I would still vote to have a consistent API and use className={classNames(classes.root, classNameProp)} to allow <EveryMuiComponent className="c1" classes={{ root: 'c2' }} /> which is perfectly valid for Button and will be equivalent to <ButtonBase className="c1 c2" /> but for SpeedDialAction this would only be <Tooltip className="c1" />.

So I would always "suffer" the increased API surface if this increases API consistency.

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 22, 2018

Choose a reason for hiding this comment

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

This change reverts a pull request I have merged too quickly without waiting for the visual regression tests. It was breaking the Speed Dial.
It's how I have discovered the Tooltip property priority issue.

For the SpeedDial, IMHO we should rename the button classes root. But I think that it's for another pull request. Here, I focus on the tooltip and going back to the previous state of affaire.

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 22, 2018

Choose a reason for hiding this comment

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

but for SpeedDialAction this would only be .

It should be c1 c2 with the current implementation. The className isn't applied on the Tooltip. #13634

<h1 className="bar">H1</h1>
</Tooltip>,
);
assert.strictEqual(wrapper.find('h1').props().className, 'bar');
Copy link
Member

Choose a reason for hiding this comment

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

Has nothing to do with this PR but this is why forwarding/merging props with cloneElement is always dangerous. At least by looking at it it might be obvious that foo will be ignored but you could also argue that this warrants a special merge strategy for the props (i.e. concatenating classes). If the child props are opaque then this will be less obvious e.g. the child is another component without passed props that then renders the h1 with a different class and you're then debugging why foo is not applied in the DOM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, let's merge the class names. I believe it's what we do by "default" in the other components.

@oliviertassinari oliviertassinari force-pushed the tootlip-fix-properties-order branch from 21737e3 to 3071343 Compare November 22, 2018 18:34
@oliviertassinari oliviertassinari merged commit c7a1dcd into mui:master Nov 22, 2018
@oliviertassinari oliviertassinari deleted the tootlip-fix-properties-order branch November 22, 2018 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work 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.

2 participants