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

[react] Add comment about removed index signature #24939

Merged

Conversation

frenic
Copy link
Contributor

@frenic frenic commented Apr 12, 2018

The index signature for React.CSSProperties was removed to enable closed typing. In this PR I just added a comment about that and how to use module augmentation to add CSS custom properties or a index signature of their own.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).
  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@frenic
Copy link
Contributor Author

frenic commented Apr 12, 2018

cc @pelotom

@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Apr 12, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 12, 2018

@frenic Thank you for submitting this PR!

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @morcerf @tkrotoff @DovydasNavickas @onigoetz @theruther4d @guilhermehubner @JoshuaKGoldberg @jrakotoharisoa - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

/**
* The index signature was removed to enable closed typing for style
* using CSSType. You're able to use module augmentation to add other
* properties, like CSS Custom Properties, or add a index signature
Copy link
Contributor

Choose a reason for hiding this comment

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

“...or add an* index signature...”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 👍

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Apr 12, 2018
@typescript-bot
Copy link
Contributor

@frenic One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

* [index: string]: any;
* }
* }
* ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also good to mention that a cheap alternative, esp. for missing properties where you don't need to reference it in multiple places, is casting:

<div style={{ ['overflowAnchor' as any]: 'auto' }}>
  ...
</div>

Copy link
Contributor Author

@frenic frenic Apr 12, 2018

Choose a reason for hiding this comment

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

I added that to the README at csstype. Thanks 👍

@pelotom
Copy link
Contributor

pelotom commented Apr 12, 2018

How about adding a comment to the effect of "If you find that these typings are missing a valid CSS property or value, please check whether an issue has already been filed for it at https://github.com/frenic/csstype/issues and if not, create one."

@frenic frenic force-pushed the removed-index-signature-comment branch from 2a77093 to 254454b Compare April 12, 2018 20:37
@frenic
Copy link
Contributor Author

frenic commented Apr 12, 2018

@pelotom I changed the comment a bit and added a link to the README of csstype where they can find exampes and more information. It's easier to maintain that README rather than this comment.

@typescript-bot typescript-bot added Other Approved This PR was reviewed and signed-off by a community member. and removed Revision needed This PR needs code changes before it can be merged. labels Apr 12, 2018
@frenic frenic force-pushed the removed-index-signature-comment branch from 254454b to 6da4210 Compare April 12, 2018 21:50
@johnnyreilly johnnyreilly merged commit 3b4991e into DefinitelyTyped:master Apr 13, 2018
@johnnyreilly
Copy link
Member

Thanks!

@frenic frenic deleted the removed-index-signature-comment branch April 13, 2018 06:45
@AdamTReineke
Copy link
Contributor

AdamTReineke commented Apr 24, 2018

This helped me debug a compile error today after updating React, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Other Approved This PR was reviewed and signed-off by a community member. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants