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

Add types to Tip #26173

Merged
merged 3 commits into from
Oct 21, 2020
Merged

Add types to Tip #26173

merged 3 commits into from
Oct 21, 2020

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Oct 15, 2020

Description

  • Add missing ReactNode type to be able to properly type children
  • Type the Tip component

How has this been tested?

Local typecheck.

Types of changes

New types.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

/cc @sirreal

Comment on lines -2 to +4
* Internal dependencies
* WordPress dependencies
*/
import { SVG, Path } from '../';
import { SVG, Path } from '@wordpress/primitives';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to make it possible to add types to Tip. Is there a reason why we weren't importing this directly from primitives?

@sirreal sirreal requested review from sirreal and ockham October 15, 2020 22:04
@sirreal sirreal added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components npm Packages Related to npm packages labels Oct 15, 2020
"src/dashicon/*",
"src/tip/*"
],
"references": [ { "path": "../primitives" } ],
Copy link
Member

Choose a reason for hiding this comment

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

We have to add references so TypeScript can figure out the build order properly. It’s the one “gotcha” to the setup

We would want this for all the monorepo packages this package depends on, but we have a problem where this isn't a "leaf" package so we need to add them as we need them.

This should fix the test issues that appeared on this branch.

Copy link
Member

Choose a reason for hiding this comment

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

We need to add one for @wordpress/element as well. We could add all the packages that have opted-in to typing and appear in this package`s package.json.

Comment on lines 43 to 48
/**
* Object containing a React node.
*
* @typedef {import('react').ReactNode} WPNode
*/

Copy link
Member

Choose a reason for hiding this comment

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

I'd vote for dropping the aliases and using the react types directly. There's no good way to re-export TS types via JavaScript and this isn't going to be sustainable.

@sarayourfriend sarayourfriend merged commit 19f2ee7 into WordPress:master Oct 21, 2020
@sarayourfriend sarayourfriend deleted the add/types-to-tip branch October 21, 2020 17:54
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants