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

[typescript] Fix for Popover -> PaperProps typing #8129

Merged
merged 1 commit into from Sep 11, 2017
Merged

[typescript] Fix for Popover -> PaperProps typing #8129

merged 1 commit into from Sep 11, 2017

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Sep 10, 2017

This was failing in ts:

 <Popover
  PaperProps={{
    style: {
      backgroundColor: 'rgb(30, 30, 30)'
    }
  }}
  >
    {tooltip}
  </Popover>

@xaviergonz xaviergonz changed the title fix for Popover -> PaperProps TS typing [typescript] Fix for Popover -> PaperProps typing Sep 10, 2017
@sebald
Copy link
Member

sebald commented Sep 11, 2017

@xaviergonz Can you please add a test for the Popover to test/typescript/components.spec.tsx. You can just copy/pase your failing example at the end!

@@ -24,7 +24,7 @@ export type PopoverProps = {
transformOrigin?: Origin;
transitionDuration?: number | 'auto';
theme?: Object;
} & Partial<TransitionHandlers> &
PaperProps;
PaperProps?: Partial<PaperProps>;
Copy link
Member

Choose a reason for hiding this comment

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

Partial is not needed, but also doesn't hurt, I guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to be future proof in case paper ever gets a required prop :)

Copy link
Member

Choose a reason for hiding this comment

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

Good thinking.

@oliviertassinari oliviertassinari merged commit bdb124b into mui:v1-beta Sep 11, 2017
@oliviertassinari
Copy link
Member

@xaviergonz I was waiting for a test case, but in the end, it's always better to have this fix than nothing. Thank you :)

@xaviergonz
Copy link
Contributor Author

Sorry, I didn't have the time to, just got home from work :-S

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2017

@xaviergonz I understand that it can be hard to find some time to contribute. It's already a step forward. I don't think typing tests are as important as unit tests for our component as the checks are static, you have a very kick feedback loop between breaking one and fixing it. The main issue is around shipping a broken typescript to the users. So, having some is a nice to have :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants