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 a HOC to provide a onChange without name required #23

Closed
gtnx opened this issue Oct 10, 2018 · 3 comments
Closed

Add a HOC to provide a onChange without name required #23

gtnx opened this issue Oct 10, 2018 · 3 comments

Comments

@gtnx
Copy link

gtnx commented Oct 10, 2018

We may use realue awesome HOCs on top of third parties components which have different various onChange signatures.

The simplest case i can think is onChange(value). For instance, this is the case for react-datetime or react-bootstrap-typeahead. In that case, we add the same hoc:

const withRealueOnChange = withHandlers({
  onChange: ({ onChange, name }) => value => onChange(value, name),
})

Several ideas:

  • Move onChange signature to onChange(value, payload).
  • property and item hoc provide on additional props onChangeSimple
  • Handle the case where name is undefined and use by the default the one from props

What do you think?

@davidbonnet
Copy link
Member

Great suggestion!

Indeed, we need an adapter that includes the name as second argument, akin fromEvent:
https://github.com/davidbonnet/realue/blob/e2cd5401266c1daff820fa0ca593238c3da1afa9/src/dom.js#L24-L33)

Could be called fromValue:

export const fromValue = memoize(path => {
  return branch(
    hasProp('onChange'),
    withHandlers({
      onChange: ({ onChange, name }) => value =>
        onChange(path == null ? value : get(value, path), name),
    }),
  )
})

@gtnx
Copy link
Author

gtnx commented Dec 19, 2018

Sure we can do that but I still don't understand why the name is required in the onChange prop provided by object hoc.

In other words, what would be the cost of turning this code:

https://github.com/davidbonnet/realue/blob/e49585c8e50d3bf7f0e914cc7de5333112afdec6/src/objects.js#L28-L38

into

  withHandlers({
    property: ({ value, onChangeProperty }) => (
      name,
      key = name,
    ) => ({
      value: value[name],
      key,
      name,
      onChange: (value, payload) => onChangeProperty(value, name, payload),
    }),
  }),

@davidbonnet
Copy link
Member

@gtnx: The cost is similar to assigning a function expression to a prop: it updates at each render.

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

No branches or pull requests

2 participants