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

RFC: Base component #419

Closed
levithomason opened this issue Aug 23, 2016 · 7 comments
Closed

RFC: Base component #419

levithomason opened this issue Aug 23, 2016 · 7 comments

Comments

@levithomason
Copy link
Member

levithomason commented Aug 23, 2016

There is basic functionality that is shared by all components:

  • merge props className with SUI classes
  • spread unhandled props
  • get element type (augmentation)
  • define as and className propTypes, most the time also children

Updating components in any of these areas requires updating all components. This was just done to support augmentation, see #414. There are ~1,800 lines added to accomplish this. The grand majority of this work was duplicating the logic and propTypes for all components.

It seems we could use a base component factory to generate a "conformant" base component. It would handle the common functionality for all components. It could make maintenance much easier. It could also give us the ability to better standardize how components are generated.

This would either work similar to the reusable component parts factories in #345, or be a base component that we extend. In either case, the above list of required features would be abstracted into a single place.

@jeffcarbs
Copy link
Member

I've been doing some cleanup lately and I noticed this as well. Not sure the best way forward here, but just wanted to bump this thread.

So many of our components, specifically our sub-components, are nearly identical. Here's an example:
screen shot 2016-10-04 at 3 51 12 am

-function MessageContent(props) {
+function MessageHeader(props) {
   const { children, className } = props
-  const classes = cx('content', className)
-  const rest = getUnhandledProps(MessageContent, props)
-  const ElementType = getElementType(MessageContent, props)
+  const classes = cx('header', className)
+  const rest = getUnhandledProps(MessageHeader, props)
+  const ElementType = getElementType(MessageHeader, props)

   return <ElementType {...rest} className={classes}>{children}</ElementType>
 }

-MessageContent._meta = {
-  name: 'MessageContent',
+MessageHeader._meta = {
+  name: 'MessageHeader',
   parent: 'Message',
   type: META.TYPES.COLLECTION,
 }

-MessageContent.propTypes = {
+MessageHeader.propTypes = {
   /** An element type to render as (string or function). */
   as: customPropTypes.as,

   /** Primary content. */
-  children: customPropTypes.children(MessageContent),
+  children: customPropTypes.children(MessageHeader),

   /** Additional classes. */
   className: PropTypes.node,
 }

-export default MessageContent
+export default MessageHeader

Literally the only difference (aside from the component name) is the class name applied.

@levithomason
Copy link
Member Author

Yep, for this I've been toying with "reusable component parts", see #345. I made a memoized component part factory, createPartFactory. It creates a class that handles the case you raised, where only the name and className and some _meta info change.

The "base component" I'm thinking of would also be usable in any of our components somehow. Example, maybe it has getClasses and getElementType methods. It could define the most basic propTypes as well, like as and className. The downsides I see are that it means all other components should extend it and therefore they should all be classes. Also, we'd have to rework the docs considerably as they do not parse inheritance or even spreading props.

I'm not really sure if this is a great idea, a base component. Though, the reusable parts for the most basic component parts (sub components) I think surely is.

@hallister
Copy link

hallister commented Dec 17, 2016

There's already an idiomatic way of doing this with an Higher Order Components.

export default baseComponent(WrappedComponent, class) {
    return class BaseComponent extends React.Component {
        displayName = WrappedComponent.displayName || 'BaseComponent';

        render() {
           const { className, children, ...props } = this.props;
           const classes = cx(class, className)
           const rest = getUnhandledProps(WrappedComponent, props)
           const ElementType = getElementType(WrappedComponent, props)
           
           return <WrappedComponent {...rest} className={classes} component={ElementType}>{children}</WrappedComponent>
        }
   }
};

Now your sub-components are vastly simpler:

function MessageHeader(props) {
    const { component: Component, ...rest } = props;
    return <Component {...rest} />;
}

MessageHeader._meta = {
   name: 'MessageHeader',
   parent: 'Message',
   type: META.TYPES.COLLECTION,
}

export baseComponent(MessageHeader, 'header');

@cdaringe
Copy link
Contributor

cdaringe commented Apr 20, 2017

  • @hallister, in your example, should styles perhaps also receive first class treatment? stlyes={defaultsDeep({}, props.styles, componentStyles)}
  • i'm still grokking handledProps (xref). i'm wondering why it's a build time process step to generate those vs having a fixed set of prop keys that are supported to be passed down the tree vs a runtime step? a runtime step would be really rad to increase portability, if at all possible

@levithomason
Copy link
Member Author

At least for SUIR right now, there are no styles until #1579 lands.

handledProps is a build time step because propTypes are stripped in production. They are wrapped so that they are removed with dead code elimination. This means, at runtime, you have no interface that tells you which props the component handled.

@sebilasse
Copy link

cc. Semantic-Org/Semantic-UI#5420 - while working on a dojo2 integration I also noticed that a shared baseClass for Container and Grid might make sense.
Currently I can set the Container properties in Grid but not vice versa.

@levithomason
Copy link
Member Author

Closing for housekeeping.

I have a branch going which will add popup and tooltip props to all components that support can support them. This will introduce the idea of HOCs for library features.

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

No branches or pull requests

5 participants