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

Fields depending on fields (feature proposal) #841

Closed
nygardk opened this issue Apr 21, 2016 · 21 comments
Closed

Fields depending on fields (feature proposal) #841

nygardk opened this issue Apr 21, 2016 · 21 comments
Labels
discussion enhancement improvements in current API
Milestone

Comments

@nygardk
Copy link

nygardk commented Apr 21, 2016

I used to do e.g. this in v5:

{!!fields.showInfo && (
  <input {...fields.info} />
)}

Now in v6 must do this (that I understand):

<Field name="showInfo" component={showInfo => !!showInfo.value && (
  <Field name="info" component={info => (
    <input {...info} />
  )} />
)} />

IMO it becomes a bit messy, especially when it is any more complicate than that.

My proposal would be to use a Fields component for convenience:

<Fields names=["showInfo", "info"] component={fields => !!fields.showInfo && (
  <input {...fields.info} />
)} />

Any opinions or plans on this?

@erikras
Copy link
Member

erikras commented Apr 21, 2016

Good point and good use case. Yes, that seems like something that needs to be addressed.

@sebfek
Copy link

sebfek commented Apr 25, 2016

Uhm very good point! I met the same problem today.

@davidkpiano
Copy link

@erikras is there any way to reveal fields from the Redux store? I imagine this would be the easiest way of handling this rather than adding ad-hoc features:

// in connected component's render
const { fields } = this.props;

return (
  { fields.showInfo.value &&
    <Field name="info" component={...} />
  }
);

@erikras
Copy link
Member

erikras commented Apr 25, 2016

@davidkpiano And how does that not require a rerender of the entire store on every change?

@davidkpiano
Copy link

@erikras IMO it's the responsibility of the developer to select the relevant pieces of data that they might need, in connect(selector)(MyForm), so that it only re-renders when they need it to re-render.

@erikras
Copy link
Member

erikras commented Apr 25, 2016

Fair enough. Seems like the only tenable and flexible solution. 👍

@erikras
Copy link
Member

erikras commented Apr 26, 2016

@nygardk, @aight8, how would you feel about something like this? Special selectors to help you connect to specific parts of the redux-form state:

import { formValue, isValid, reduxForm } from 'redux-form';

@connect(
  state => ({
    showInfoValue: formValue('myForm', 'showInfo')
    itemAValid: isValid('myForm', 'itemA', myValidation)
    itemBValid: isValid('myForm', 'itemB', myValidation)
  })
)
@reduxForm({
  form: 'myForm',
  validate: myValidation
})
class MyForm extends Component {
  render() {
    const { showInfoValue, itemAValid, itemBValid } = this.props;
    return (
      <div>
        <Field name="itemA" component={React.DOM.input} type="text"/>
        <Field name="itemB" component={React.DOM.input} type="text"/>
        <div className={!itemAValid || !itemBValid}>A or B is invalid</div>
        <Field name="showInfo" component={React.DOM.input} type="checkbox"/>
        {showInfoValue && <Field name="info" component={React.DOM.input} type="text"/>}
      </div>
    );
  }
}

Make sense?

@aight8
Copy link

aight8 commented Apr 26, 2016

I notice some points:
The myForm, and isValid selector has the form name hardcoded.
Additionally I can't connect the MyForm's root div to get my extra infos since the form name is unknown there (I must pass it manually down and it's were a little bit a mess to connect directly divs - that's become obscure)

The nice thing is that the info are accessible directly in the prop and the form is only rendered when the very specific piece of information will change.


Back to the original thought. When I can just add some additional field names (maybe in another prop?) to the Field component and it will observe it for me with the correct form name as target it takes the whole work and it's connected at the deepest level where it's relevant. The downside: The whole Field component is re-rendered every time when one of the given field's changes any property.

The best performance were:

<Field name="myField" observeFieldsValid={['myInfo', 'blaBlebb']} observeFieldsValues={['otherField', 'testField']} component={...} />

But than the Field component needs extra props for type of prop.

So this is way easier (but it listen to all changes):

<Field name={['myField', 'myInfo']} />

So maybe something like this?:

<Field name={'info'} observe={{isValid: ['bla', 'myinfo'], value: ['test', 'myinfo']}} />

Then access inside Field component like this:

this.props.observe.isValid.bla
this.props.observe.value.myinfo

I want to prevent this redux connect hell (imagine I have nested component's) and the fact it should determinate the form name directly.

@erikras
Copy link
Member

erikras commented Apr 26, 2016

@aight8 Good thoughts. Will think about it. My initial reaction is that the observe API is needlessly verbose and complex. <Field name={['a', 'b']}/> feels the least worst.

@nygardk
Copy link
Author

nygardk commented Apr 27, 2016

@aight8 , I'm with @erikras about the observe API: it would be rather complex and confusing as it essentially connects the other fields but in a completely different way.

What are the negatives of <Field name={['a', 'b']}/>? I think the only confusing part about it is that it would have to provide a fields object as an argument for the component function, which is different from the "normal" behavior. On the other hand, I see it as a very common use case so it would be justified.

@aight8
Copy link

aight8 commented Apr 27, 2016

Yap, I already tought so that my idea were too complicated but wanted to write my toughts down.

The <Field name={['a', 'b']}/> seems pritty nice (although the prop name and it's context is confusing then for new-comers. At the first sight on this it makes not so much sense semantically - one Field with a list of names as name?).
It's not really a issue but remember that when you add additional field names to the array and those other fields change (focus, valuechange, blur etc.) the current Field component will be rerended everytime, even when you just looking a specific part of the other fields. But I don't think that will become an issue.

@ooflorent ooflorent added enhancement improvements in current API discussion labels Apr 28, 2016
@erikras erikras added this to the next-6.0.0 milestone May 12, 2016
@erikras
Copy link
Member

erikras commented May 16, 2016

Published my "form value selector" API as v6.0.0-alpha.11. Feedback welcome!

Note that this does not alleviate the need for the <Fields name={['a', 'b']}/> API, so I'm not closing this ticket.

@tnrich
Copy link
Contributor

tnrich commented May 28, 2016

Any movement on this one? I'm not sure I understand what the benefits of the <Fields name={['a', 'b']}/> component is..? Doesn't it get us into the same trouble as v5 where the inputs are rerendering on every change? Also, it seems to encourage the idea that if you have connected fields, put them together in the same component, but that seems less modular than the individual Field approach with selectors for values and errors and other meta data. Just my two cents, I could be missing something.

@alurim
Copy link
Contributor

alurim commented Jun 17, 2016

Didn't read this until yesterday when I posted #1173. Agree with @nygardk that expanding the API would really make such use cases better!

@alurim
Copy link
Contributor

alurim commented Jun 17, 2016

I'm sure this can be more performant if it's built into the library instead of composing Fields all the way down, but if you're looking to try out this API, this will get the job done.

7/13 Edited the code to what I'm currently using:

import React, { PropTypes } from 'react'
import { Field } from 'redux-form'
import _ from 'lodash'

const RecursiveField = ({
    active,
    asyncValidating,
    checked,
    dirty,
    error,
    invalid,
    name,
    onBlur,
    onChange,
    onDragStart,
    onDrop,
    onFocus,
    onUpdate,
    pristine,
    touched,
    valid,
    value,
    visited,
    __path,
    __names,
    __component,
    __otherProps,
    __fieldProps,
}) => {
    const fieldProps = {
        ...__fieldProps,
        [name.replace(__path, '')]: {
            active,
            asyncValidating,
            checked,
            dirty,
            error,
            invalid,
            name,
            onBlur,
            onChange,
            onDragStart,
            onDrop,
            onFocus,
            onUpdate,
            pristine,
            touched,
            valid,
            value,
            visited,
        },
    }
    if (__names.length === 0) {
        return (
            <__component
                {...__otherProps}
                {...fieldProps}
            />
        )
    }
    return (
        <Field
            name={`${__path}${_.head(__names)}`}
            component={RecursiveField}
            __path={__path}
            __names={_.tail(__names)}
            __component={__component}
            __otherProps={__otherProps}
            __fieldProps={fieldProps}
        />
    )
}

RecursiveField.propTypes = {
    active: PropTypes.bool,
    asyncValidating: PropTypes.bool,
    checked: PropTypes.bool,
    dirty: PropTypes.bool,
    error: PropTypes.string,
    invalid: PropTypes.bool,
    name: PropTypes.string,
    onBlur: PropTypes.func,
    onChange: PropTypes.func,
    onDragStart: PropTypes.func,
    onDrop: PropTypes.func,
    onFocus: PropTypes.func,
    onUpdate: PropTypes.func,
    pristine: PropTypes.bool,
    touched: PropTypes.bool,
    valid: PropTypes.bool,
    value: PropTypes.any,
    visited: PropTypes.bool,
    __path: PropTypes.string,
    __names: PropTypes.arrayOf(PropTypes.string),
    __component: PropTypes.func,
    __otherProps: PropTypes.object,
    __fieldProps: PropTypes.object,
}

const Fields = ({ path = '', names, component, ...otherProps }) => {
    return (
        <Field
            name={`${path}${_.head(names)}`}
            component={RecursiveField}
            __path={path}
            __names={_.tail(names)}
            __component={component}
            __otherProps={otherProps}
        />
    )
}

Fields.propTypes = {
    path: PropTypes.string,
    names: PropTypes.arrayOf(PropTypes.string),
    component: PropTypes.func,
}

export default Fields

@mike1808
Copy link
Contributor

@erikras thanks for that solution. It really solved an issue with dependant fields.
Also, if you want to change the Field value you can use built-in action creators change.

@sebinsua
Copy link

sebinsua commented Jul 7, 2016

I am also wondering what the best way of doing this is.

My use-case is an array of <select> boxes, which when changed affect the label and unit of a <input type="text"> associated with each of them.

@sebinsua
Copy link

sebinsua commented Jul 8, 2016

I hacked together some logic for my use-case. Not contributable since I think the private redux-form API and the interface design in the OP would make a much better implementation.

However, I'll share it here since it might be useful to others.

Example: passing a single property into some inner <Field>s.

<FieldGroup name="inputs.measurementUnit">
  <Field
    name={`inputs.amount`}
    component={renderInputAmount}
  />
</FieldGroup>

Example: you can also pass in multiple properties from a single object, like so:

<FieldGroup name="inputs" expose={['measurementUnit', 'currency']}>
  <Field
    name="inputs.amount"
    component={renderInputAmount}
  />
  <Field
    name="inputs.cost"
    component={renderInputCost}
  />
</FieldGroup>

And, additionally if you need to transform property keys then you can pass in { from, to } to expose.

Here is the implementation:

import React, { Children, PropTypes, isValidElement, cloneElement } from 'react'
import { Field } from 'redux-form'

import isPlainObject from 'lodash/isPlainObject'
import merge from 'lodash/merge'
import flow from 'lodash/flow'
import some from 'lodash/fp/some'
import find from 'lodash/fp/find'
import pick from 'lodash/fp/pick'
import transform from 'lodash/transform'
import identity from 'lodash/identity'
import kebabCase from 'lodash/kebabCase'

const exposeValuePropType = PropTypes.oneOfType([
  PropTypes.string,
  PropTypes.shape({
    from: PropTypes.string.isRequired,
    to: PropTypes.string.isRequired
  })
])
const exposePropType = PropTypes.oneOfType([ PropTypes.arrayOf(exposeValuePropType), exposeValuePropType ])

function createTransformWithExpose (exposeArr) {
  return function transformWithExpose (values) {
    return transform(values, (newValue, valueValue, valueKey) => {
      const match = find(exposeItem => exposeItem.from === valueKey, exposeArr)
      const toKey = match ? match.to : valueKey
      newValue[toKey] = valueValue
      return newValue
    }, {})
  }
}

function createExposeValueAsProps (groupName, expose) {
  // Read down...
  // The constants defined here are units of functionality used within exposeValueAsProps.

  const exposeArr = [].concat(expose) // We default to exposing a list of properties.
  const exposeKeys = exposeArr.map(exposeItem => isPlainObject(exposeItem) ? exposeItem.from : exposeItem)

  const pickPropsFromValueObject = exposeKeys.length > 0 ? pick(exposeKeys) : identity

  const toPropsFromValue = (fieldName) => {
    const propertyName = fieldName.split('.').slice(-1).pop()
    return (value) => ({
      [`${propertyName}`]: value
    })
  }

  const exposeHasTransforms = some(isPlainObject, exposeArr)
  const transformKeys = exposeHasTransforms ? createTransformWithExpose(exposeArr) : identity

  const getTransformedPropsOfValueObject = flow([ pickPropsFromValueObject, transformKeys ])
  const getTransformedPropsOfValue = flow([ toPropsFromValue(groupName), transformKeys ])

  return function exposeValueAsProps (value) {
    if (isPlainObject(value)) {
      // get the relevant props from a value object
      return getTransformedPropsOfValueObject(value)
    }

    // else, construct the props from a singular value, if it's truthy
    return value ? getTransformedPropsOfValue(value) : {}
  }
}

function isNotDom (child) {
  return typeof child.type !== 'string'
}

function isReduxFormField (child) {
  const componentName = typeof child.type === 'string' ? child.type : child.type.displayName || child.type.name
  return ['Field', 'FieldArray'].includes(componentName)
}

function placePropsOntoChildren (children, extraProps, shouldPlaceExtraPropsOntoChild = isNotDom) {
  return Children.map(
    children,
    child => {
      if (!isValidElement(child)) {
        return child
      }

      const newChildProps = {}
      if (child.props && child.props.children) {
        newChildProps.children = placePropsOntoChildren(child.props.children, extraProps, shouldPlaceExtraPropsOntoChild)
      }
      return cloneElement(child, shouldPlaceExtraPropsOntoChild(child) ? merge(newChildProps, extraProps) : newChildProps)
    }
  )
}

function renderChildrenWithGroupsProps ({
  input: {
    value,
    className,
    groupName,
    expose,
    children
  }
}) {
  // Take the field group name and a list of properties you wish to expose
  // and place each of these into the props of the non-DOM, redux-formy children within
  // the current FieldGroup while transforming the key names as necessary.
  const exposeValueAsProps = createExposeValueAsProps(groupName, expose)
  const childrenWithProps = placePropsOntoChildren(children, exposeValueAsProps(value), isReduxFormField)
  return (
    <div className={`field-group ${className}`}>
      {childrenWithProps}
    </div>
  )
}

renderChildrenWithGroupsProps.propTypes = {
  input: PropTypes.shape({
    value: PropTypes.any.isRequired,
    className: PropTypes.string.isRequired,
    expose: exposePropType,
    children: PropTypes.node.isRequired
  }).isRequired
}

// NOTE: This can be removed if a feature to pass fields dependent state is added upstream.
// See: https://github.com/erikras/redux-form/issues/841
export function FieldGroup ({
  name,
  expose,
  children
}) {
  return (
    <Field
      name={name}
      component={renderChildrenWithGroupsProps}
      className={`field-group-${kebabCase(name)}`}
      groupName={name}
      expose={expose}
      children={children}
    />
  )
}

FieldGroup.propTypes = {
  name: PropTypes.string.isRequired,
  expose: exposePropType,
  children: PropTypes.node.isRequired
}

export default FieldGroup

Also as a module, here: redux-form-field-group

@erikras
Copy link
Member

erikras commented Aug 19, 2016

A Fields component was published in v6.0.0-rc.5 that does just this.

@erikras erikras closed this as completed Aug 19, 2016
@nygardk
Copy link
Author

nygardk commented Aug 19, 2016

@erikras awesome!

@lock
Copy link

lock bot commented Jun 2, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion enhancement improvements in current API
Projects
None yet
Development

No branches or pull requests

10 participants