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 TS types for component-component, dialog, portal, rect, alert-dialog, and utils. #105

Closed
wants to merge 7 commits into from

Conversation

hedgerh
Copy link

@hedgerh hedgerh commented Jan 13, 2019

Update - All typedefs are being published to DefinitelyTyped. Read on:

To install them, you have to use a special syntax. For example:

yarn add @types/reach__menu-button
yarn add @types/reach__alert-dialog
yarn add @types/reach__tabs

etc.

Some have been published, some are in review. Follow along here - #37 (comment)

@hedgerh hedgerh mentioned this pull request Jan 13, 2019
@hedgerh hedgerh changed the title [WIP] Add TS types for @reach/component-component [WIP] Add TS types for component-component, alert-dialog, dialog, portal, rect Jan 13, 2019
@hedgerh hedgerh changed the title [WIP] Add TS types for component-component, alert-dialog, dialog, portal, rect [WIP][RFC] Add TS types for component-component, dialog, portal, rect, alert-dialog, and utils. Jan 13, 2019
@hedgerh hedgerh changed the title [WIP][RFC] Add TS types for component-component, dialog, portal, rect, alert-dialog, and utils. [RFC][WIP] Add TS types for component-component, dialog, portal, rect, alert-dialog, and utils. Jan 13, 2019
@Miklet
Copy link
Contributor

Miklet commented Jan 13, 2019

One thing that caught my eye is using I as the prefix of every interface name. In my opinion it's unnecessary. Also, as I remember correctly, none of already defined typings use this convention 🙂 I will look more into it when I will have access to my computer.

@hedgerh
Copy link
Author

hedgerh commented Jan 13, 2019

Ahh yeah I'll take them out! I'm about to make another pass over everything and fix a couple of things.

@Miklet
Copy link
Contributor

Miklet commented Jan 13, 2019

Awesome 👍

@hedgerh
Copy link
Author

hedgerh commented Jan 13, 2019

pushed up a couple of changes. there are a few things im not sure about that i was going to leave some discussions on.

@hedgerh hedgerh changed the title [RFC][WIP] Add TS types for component-component, dialog, portal, rect, alert-dialog, and utils. Add TS types for component-component, dialog, portal, rect, alert-dialog, and utils. Jan 22, 2019
@hedgerh
Copy link
Author

hedgerh commented Jan 22, 2019

Removed the RFC and WIP tags from this. It's ready for review.

@ecoleman
Copy link

@hedgerh Can you submit these to Definitely Typed?

@hedgerh
Copy link
Author

hedgerh commented Mar 22, 2019

@ecoleman once they get reviewed, im hoping they will just get merged in here so that they wont need to be published to Definitely Typed, but im open to it. im going to pull them back down and test them to see if they're good. i am hoping someone who is more adept with Typescript can give them a look over.

Harry Hedger added 2 commits March 30, 2019 12:14
@hedgerh hedgerh force-pushed the types-for-component branch 3 times, most recently from f265ece to e4a0a4e Compare March 30, 2019 20:14
@johnridesabike
Copy link

I just copied the Tabs typedefs to my project to test it out, and ran into a few bugs:

  • Props like className and style aren't included, even though they're valid props. There may be other standard props that could be added, too.
  • children is defined as a required prop, but it technically isn't required. A good example of when you wouldn't use children is when you're making a custom tab like in the documentation: const RedTab = props => <Tab {...props} style={{ color: "red" }} />

@hedgerh
Copy link
Author

hedgerh commented Apr 20, 2019

Hey hey, I'll make the updates real quick. I pulled those in from someone else who had typed them. I'm not really sure what to do with this MR. It's been sitting in limbo forever.

@hedgerh
Copy link
Author

hedgerh commented Apr 20, 2019

@johnridesabike after reviewing the types that are on my branch, it appears that children is optional on the Tab component:

  export interface BaseTabProps<A> {
    children?: React.ReactNode;
    as?: ComponentType;
    rest?: A;
  }

  export interface TabProps<A = React.HTMLAttributes<HTMLDivElement>>
    extends BaseTabProps<A> {
    disabled?: boolean;
  }

As for className and style, are there absolutely no typings around them? Typically they get typed because we extend our prop interfaces with React.HTMLAttribute<HTMLWhateverElement> and that includes those types of props. It doesn't look like TabProps is extending it here. Let me know if it actually types them. If it doesn't, I'll make sure to explicitly extend it. Thanks.

@johnridesabike
Copy link

@hedgerh I think you're right about the children. I don't know why that popped up for me... maybe I was still using the other typedefs I copied?

className and style still don't seem to work though. Here's the TS error:

Type '{ children: Element[]; className: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Tabs> & Readonly<TabsProps<HTMLAttributes<HTMLDivElement>>> & Readonly<{ children?: ReactNode; }>'.
  Property 'className' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Tabs> & Readonly<TabsProps<HTMLAttributes<HTMLDivElement>>> & Readonly<{ children?: ReactNode; }>'.

@swyxio
Copy link

swyxio commented Apr 20, 2019

hi @hedgerh thanks for posting your types just wanna add that @reach/rect should also export this:

export function useRect(rect: Ref, isSelected?: boolean): IClientRect

i also use IClientRect a lot (e.g. in a context), so i'd like to export that too and give it a nicer name. this is my .d.ts file

declare module "@reach/rect" {
  import { Ref } from "react"
  export interface Rect {
    x: number
    y: number
    width: number
    height: number
    top: number
    right: number
    bottom: number
    left: number
  }

  type RectProps = {
    observe?: boolean
    onChange?: (rect: Rect) => void
    children?: React.ReactNode
  }

  const Rect: React.SFC<RectProps>

  export default Rect
  export function useRect(rect: Ref, isSelected?: boolean): Rect
}

@swyxio
Copy link

swyxio commented Apr 20, 2019

@reach/tabs BaseTabProps should also have a style:

  import { ComponentType, StyleHTMLAttributes } from "react"

  export interface BaseTabProps<A> {
    children?: React.ReactNode
    as?: ComponentType
    rest?: A
    style?: StyleHTMLAttributes // missing from hedgerh
  }

@hedgerh
Copy link
Author

hedgerh commented Apr 20, 2019

hey @sw-yx, thanks for the callouts. Is BaseTabProps even typed right at all? rest isn't an actual prop on Tabs. It's the props leftover from using the object spread property syntax.. Any idea if that's legit? I think that's why you all are having issues with className and style, which are covered by HTMLAttributes. Can one of you try passing the Tabs some props that correlate to attributes that can be passed to a div?

Harry Hedger and others added 3 commits April 20, 2019 17:17
@hedgerh
Copy link
Author

hedgerh commented Apr 21, 2019

actually was able to check myself and, yep, it wasn't typed right. fixed it, so className and style should work now.

@ryanflorence any chance you could signal boost or something to get someone to review and 👍 this? just realized this PR has been open for 3 months. you all are lovely, but I want to be freeeee

@johnridesabike
Copy link

johnridesabike commented Apr 21, 2019

I just tested the new tab typedefs in my project and it all seems to be good. Hopefully this can get reviewed/approved soon!

const Rect: React.FC<RectProps>

export default Rect
export function useRect(rect: Ref, isSelected?: boolean): ClientRect
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just working on some typedefs for rect as well and I noticed that this will return null on the first render before the elements are in the DOM. Awesome work!

@@ -0,0 +1,24 @@
declare module "@reach/rect" {
import { Ref } from "react"
interface ClientRect {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another little thing I noticed is that there's a type definition built in for DOMRect which you could use here instead of re-defining it. Here's a link to the DOMRect definition. You should be able to use it here by adding /// <reference lib="dom"/> to the top of this definition file. Here's a link to the triple slash documentation

@hedgerh
Copy link
Author

hedgerh commented May 16, 2019

@Dhalton Nice. I'll add that in.

Just want to let everyone know that I'm moving these types over to DefinitelyTyped right now. Ryan doesn't really want to deal with them until the library matures a bit. I'll share the MRs for the work when they're ready, and they should be ushered through by maintainers of DT.

@john-osullivan
Copy link

You're the bomb, @hedgerh ! Was afraid I'd have to write these up myself, thanks for sharing.

@hedgerh
Copy link
Author

hedgerh commented Jun 23, 2019

For anyone following this PR and not the reach ui types issue, I just completed writing some typedefs to publish to DefinitelyTyped. They are now in review. Can follow here #37 (comment)

@chaance
Copy link
Member

chaance commented Oct 2, 2019

Closing this PR for now, but as mentioned in #37 I am going to work on getting all types worked into the project over the coming weeks. Thanks everyone for your contributions here and on Definitely Typed!

@chaance chaance closed this Oct 2, 2019
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

9 participants