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

Feature/typescript prop exports #3448

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@rvaidya
Copy link

rvaidya commented Feb 5, 2019

  • Export props for each class that has them in the TypeScript definitions
  • Fixed a typo in dropdowns "alightRight" vs "alignRight"
  • Fixed copy/paste issues (Feedback was copy/pasted onto FormLabel and the class names were kept)
@rvaidya

This comment has been minimized.

Copy link
Author

rvaidya commented Feb 5, 2019

@@ -12,7 +12,7 @@ declare class AlertHeading<
As extends React.ReactType = 'div'
> extends BsPrefixComponent<As> {}

interface AlertProps extends React.HTMLProps<Alert> {
export interface AlertProps extends React.HTMLProps<Alert> {

This comment has been minimized.

@taion

taion Feb 5, 2019

Member

i guess we don't want the exported name for the individual modules here to be Props?

This comment has been minimized.

@rvaidya

rvaidya Feb 5, 2019

Author

tslint recomments interfaces all start with I -> IAlertProps

if that is preferable

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Feb 9, 2019

@taion , your hunch is right. The general convention in the React community, including the react package, is to use the name ComponentNameProps instead of just Props, to make imports of multiple props types easier.

@jquense

This comment has been minimized.

Copy link
Member

jquense commented Feb 9, 2019

The other pattern I've seen is to use a namespace but I'm not really familiar with what that would look like practically here

@thejohnfreeman
Copy link

thejohnfreeman left a comment

LGTM. I've tested the changes with @thejohnfreeman/react-bootstrap. Would like to see this merged and released officially.

@@ -1,7 +1,7 @@
import * as React from 'react';
import { TransitionCallbacks } from './helpers';

interface CollapseProps
export interface CollapseProps

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Feb 13, 2019

This interface is missing the id prop seen in the examples.

This comment has been minimized.

@prateekkathal

prateekkathal Feb 13, 2019

It's also missing className.

#3446

image

This comment has been minimized.

@rvaidya

rvaidya Feb 13, 2019

Author

I noticed that too but it's not listed in the react-bootstrap docs, and also seems to work in some cases without the id prop

https://react-bootstrap.github.io/utilities/transitions/

This comment has been minimized.

@prateekkathal

prateekkathal Feb 13, 2019

@rvaidya The id prop is non-mandatory. Add these please 😄

id?: string;
className?: string;

@thejohnfreeman I believe adding these 2 lines should do it 👍

This comment has been minimized.

@jquense

jquense Feb 14, 2019

Member

Shouldn't this extend the base html props instead of hand adding these?

This comment has been minimized.

@prateekkathal

prateekkathal Feb 14, 2019

@jquense Giving this one last try. Let me know if you're okay with this.... It fixes the issue. Otherwise someone else can take this in the future.

interface NavbarCollapseProps extends React.ClassAttributes<NavbarCollapse> {
  id?: string;
  className?: string;
}

declare class NavbarCollapse extends BsPrefixComponent<typeof Collapse, NavbarCollapseProps> {}

Thanks for the continued support on this 😄

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Feb 14, 2019

  • NavbarCollapse renders a Collapse with a div child, and passes all its props besides bsPrefix through to the Collapse.
  • The Collapse renders a Transition with a child render function that renders the child of Collapse (the div). Collapse takes out the className prop, passing it through to its child. It passes its remaining unknown props through to the Transition, which is imported from react-transition-group.
  • Transition subtracts its props and passes the rest back to its render child. Collapse defines a render child that passes these props through to its child (the div in this case).

So it seems that Collapse should expect to get a mix of the props it knows it uses (already listed as properties in CollapseProps), the props expected by Transition (named TransitionCallbacks, which CollapseProps already extends), and the props of its child (the div in this case, but could be anything). This last type is conceptually PropsOf<children>, but I don't know how to express that in TypeScript.

This comment has been minimized.

@jquense

jquense Feb 14, 2019

Member

ya I think what we actually need to do long term is change the API here so each component doesn't just pass through everything to it's children

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Feb 14, 2019

@prateekkathal , I'm with @jquense and think you should leave the type fixing out of this PR, especially since it's so complicated. Just getting proper exports is a great step forward.

This comment has been minimized.

@prateekkathal

prateekkathal Feb 14, 2019

Sounds good. 👍

@rvaidya Please revert the change here 😄

@jviall

This comment has been minimized.

Copy link

jviall commented Feb 15, 2019

Is a different issue/PR going to be opened for the lack of id and className on the NavbarCollapse prop type? Really looking forward to that fix.

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