The Wayback Machine - http://web.archive.org/web/20211007055105/https://github.com/Semantic-Org/Semantic-UI-React/issues/1169
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

Advertise all underlying propTypes and typings in wrapper components #1169

Open
14 tasks
levithomason opened this issue Jan 16, 2017 · 17 comments
Open
14 tasks

Advertise all underlying propTypes and typings in wrapper components #1169

levithomason opened this issue Jan 16, 2017 · 17 comments

Comments

@levithomason
Copy link
Member

@levithomason levithomason commented Jan 16, 2017

See #1163 for the inspiration here.

Some components wrap others, like a Popup wrapping a Portal. However, we do not advertise the Portal props as propTypes nor typings of the Popup. This means there is no way for users to know what props are available.

Add propTypes

I propose we explicitly list all known props in propTypes and typings of the underlying component when a component wraps another component.

Add Examples

We should then also add doc site examples showing use of these props. Example, Popup mouse enter/leave delays are not shown but are available as it composes the Portal which has mouse enter/leave delays.

Task List

Here is a brain dump of wrapper components, there may be more. @jcarbo / @layershifter feel free to update this list at will:

  • Popup -> Portal
  • Modal -> Portal
  • Confirm -> Modal
  • Select -> Dropdown
  • Radio -> Checkbox
  • Table.Footer -> Table.Header
  • Table.HeaderCell -> Table.Cell
  • Form.Button -> FormField + Button
  • Form.Checkbox -> FormField + Checkbox
  • Form.Dropdown -> FormField + Dropdown
  • Form.Input -> FormField + Input
  • Form.Radio -> FormField + Radio
  • Form.Select -> FormField + Select
  • Form.TextArea -> FormField + TextArea
@layershifter
Copy link
Member

@layershifter layershifter commented Jan 17, 2017

LGTM, I'll will make this updates in PRs that affects typings.

@levithomason
Copy link
Member Author

@levithomason levithomason commented Jan 17, 2017

Great, thanks! I've left Select / Radio unfinished on the list as I don't see all the props of their underlying components listed in their propTypes. Example, Dropdown takes a search prop so the Select propTypes should also list search.

@layershifter
Copy link
Member

@layershifter layershifter commented Feb 28, 2017

@levithomason I need there some clarification. Most of these pairs has a correct inheritance in typings, however, I need to make an additional check.

I'm also support an idea with docs, but there is problem with propTypes. I see two ways to deal with them.

TableHeaderCell.propTypes = {
  ...TableCell.propTypes,
  as: customPropTypes.as,
  children: PropTypes.node,,
}
TableHeaderCell.propTypes = {
  as: customPropTypes.as,
  children: PropTypes.node,
  collapsing: PropTypes.bool,
  icon: customPropTypes.itemShorthand,
}

I'm not sure that docs generation will support the first way. While second way will produce a tons of unnecessary work with handling of props:

function TableHeaderCell (props) {
  const { children, collapsing, icon } = props
  // ...
  return <TableCell collapsing={collapsing} icon={icon}>{children}</TableCell>
}

@levithomason
Copy link
Member Author

@levithomason levithomason commented Feb 28, 2017

Let me check on this, I saw another project that extended react-docgen to allow this exact usage as well. Perhaps that has been integrated in to react-docgen core by now.

@levithomason
Copy link
Member Author

@levithomason levithomason commented Feb 28, 2017

Indeed, we get some support for this. Adding your above snippet to the header cell, I get this output in the docgenInfo.json:

  "src/collections/Table/TableHeaderCell.js": {
    "methods": [],
    "props": {
      // all the TableHeaderCell props
    },
    "composes": [
      "./TableCell"
    ],
    "docBlock": {
      "description": "A table can have a header cell.",
      "tags": []
    }
  },

We can use the composes key to include all the props of the composed component.

One issue is that we need to update babel-plugin-transform-react-handled-props as it doesn't know how to handle the spread operator there:

Message:
    ./src/collections/Table/TableHeaderCell.js

Module build failed: TypeError: ./src/collections/Table/TableHeaderCell.js: Cannot read property 'name' of undefined
    at ./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:26:25
    at arrayMap (./node_modules/lodash/lodash.js:660:23)
    at Function.map (./node_modules/lodash/lodash.js:9571:14)
    at getObjectKeys (./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:25:27)
    at Store.AssignmentExpression (./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:47:34)
    at NodePath._call (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:76:18)
    at NodePath.call (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:48:17)
    at NodePath.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:105:12)
    at TraversalContext.visitQueue (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitSingle (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:108:19)
    at TraversalContext.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:192:19)
    at Function.traverse.node (./node_modules/babel-core/node_modules/babel-traverse/lib/index.js:114:17)
    at NodePath.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:115:19)
    at TraversalContext.visitQueue (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitMultiple (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:103:17)
    at TraversalContext.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:190:19)

This sounds like a slippery slope as well. It may be pretty tough to properly traverse the propTypes of objects imported from elsewhere. My hunch is that if it were easy, react-docgen would have included that feature rather than listing composes instead.

Another (much smaller) issue is that the resolve is also a little too tricky for my IDE and I'm guessing others' tools as well. This means unless you're using TS, your tool probably won't benefit much from this kind of composition. This should be taken lightly as tools will catch up someday, but it is worth considering.

This is a lot to manage by hand and also creates nasty low visibility dependencies that we're going to break often. At some point, updating a Portal prop is bound to cause breakages in all the other propTypes that use it.

Let's think on this a bit more. We want to reduce maintenance overhead, reduce dependencies, and increase doc visibility.

@layershifter
Copy link
Member

@layershifter layershifter commented Feb 28, 2017

One issue is that we need to update babel-plugin-transform-react-handled-props as it doesn't know how to handle the spread operator there:

I'll make an update to plugin on this week, it will ignore spreads.

@levithomason
Copy link
Member Author

@levithomason levithomason commented Feb 28, 2017

Won't this break the handledProps array? It needs to parse them to know which are handled and which are ...rest, correct?

@layershifter
Copy link
Member

@layershifter layershifter commented Feb 28, 2017

Nope.

It needs to parse

It's a kind of black magic if we speak about external imports like in my snippet. So, it will be easier to ignore spreads at now at all. Especially, in our case, when this behavior is expected.

TableHeaderCell.propTypes = {
  ...TableCell.propTypes,
  as: customPropTypes.as,
  children: PropTypes.node,
}
// will produce
TableHeaderCell.handledProps = ['as', 'children']

@layershifter
Copy link
Member

@layershifter layershifter commented Mar 8, 2017

I've checked typings of all components in list. All of them now correctly extends parents, except Confirm (#1425). babel-plugin-transform-react-handled-props was also updated 😄

@levithomason
Copy link
Member Author

@levithomason levithomason commented Mar 12, 2017

Great, so we should be able to spread the inherited propTypes as well then. Once that is done, I can update ComponentProps to parse and display the composes props value as well.

@stale
Copy link

@stale stale bot commented Feb 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@levithomason
Copy link
Member Author

@levithomason levithomason commented May 6, 2018

This really needs to be done. It bites again in #2579.

@stale
Copy link

@stale stale bot commented Aug 4, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Aug 4, 2018
@martindale
Copy link

@martindale martindale commented Sep 8, 2018

This issue should remain open (and closing issues marked as stale seems anti-productive...)

@stale stale bot removed the stale label Sep 8, 2018
@stale
Copy link

@stale stale bot commented Mar 7, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale
Copy link

@stale stale bot commented Sep 3, 2019

This issue will be closed due to lack of activity for 12 months. If you’d like this to be reopened, just leave a comment; we do monitor them!

@stale stale bot closed this Sep 3, 2019
@layershifter layershifter reopened this Sep 3, 2019
@stale stale bot removed the stale label Sep 3, 2019
@Anuragtech02
Copy link

@Anuragtech02 Anuragtech02 commented Sep 12, 2021

Hey @layershifter ! Is this issue still open? I'd love to work on this.

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

Successfully merging a pull request may close this issue.

None yet
4 participants