Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Made all functional props nullable #93

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

n-sviridenko
Copy link

I have a case when I need to clone an <*Element /> component with overriding of its properties with less priority. For example:

import React from 'react';

function SmartElement({ children }) {
  return React.cloneElement(children, { onChange: () => console.log('change'), ...children.props });
}

In this case, my onChange will not be called because of defaultProps.onChange of the *Element. So { ...{ onChange: myFunc }, ...{ onChange: noop } } will return { onChange: noop }.

A real use case:

https://github.com/callemall/material-ui/blob/v0.19.2/src/TextField/TextField.js#L462-L467

const inputProps = {
  id: inputId,
  ref: (elem) => this.input = elem,
  disabled: this.props.disabled,
  onBlur: this.handleInputBlur,
  onChange: this.handleInputChange,
  onFocus: this.handleInputFocus,
};

// ...

inputElement = React.cloneElement(children, {
  ...inputProps,
  ...children.props, // it overrides `inputProps`
  style: Object.assign(childStyleMerged, children.props.style),
});

Usage:

<TextField>
  <CardNumberElement />
</TextField>

@michelle-stripe
Copy link
Contributor

Hi!

Is there any reason you can't flip the two deconstructions? Is it because you still want children.props to override inputProps in some cases?

inputElement = React.cloneElement(children, {
  ...children.props,
  ...inputProps,
  style: Object.assign(childStyleMerged, children.props.style),
});

@n-sviridenko
Copy link
Author

Hi @michelle-stripe, it is an open source ui library and not my business logic part, unfortunately.

@michelle-stripe
Copy link
Contributor

OH, I see :(.

onBlur: noop,
onFocus: noop,
onReady: noop,
elementRef: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to include these in defaultProps anymore!

@michelle-stripe
Copy link
Contributor

Thanks for the PR! I just have one nit. We recently added the PaymentRequestButtonElement -- do you mind making these changes there as well?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Successfully merging this pull request may close these issues.

3 participants