Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

[Forms] onChange doesn't work as expected in React #198

Closed tolrodco closed 7 years ago

tolrodco commented 7 years ago

bug

Tradeshift UI version affected

v6.1.1

Expected Behavior

User opens a aside panel, clicks on select and chooses an option from the aside list. After selection, data is properly passed back to the select tag and made available to react to update the value.

Actual Behavior

Form and asides appear to function properly, however data is not available to the react app to update the value, instead the props.fields are not updated and are reset to their original state.

Steps to reproduce

Create a normal data-ts: Form tag with a select that contains an onClick: update function:

Example
form({ 'data-ts': 'Form' },
    fieldset({},
        label({},
            span({}, t('Test Select')),
            select({
                type: 'text',
                id: 'status',
                value: props.fields.status,
                onChange: function(e) {
                    props.onUpdate('status', e.target.value
                }
            },
                option({ value: 'Pending' }, 'Pending'),
                option({ value: 'Test 2' }, 'Test 2')
            )
        )
    ),
    props.edit ? Button({
        className: 'ts-primary full-width',
        onclick: function() {
            props.editCompany(props.fields);
        },
        title: t('Save Changes')
    }) : Button({
        className: 'ts-primary full-width',
        onclick: function() {
            props.createCompany(props.fields);
        },
        title: t('Save Company'),
        disabled: !props.fields.companyAccountId
    }),
    props.edit ? Button({
        className: 'ts-danger full-width',
        onclick: function() {
            props.deleteCompany(props.fields);
        },
        title: t('Delete Company')
    }) : ''
);
Aside state:
 asides: {
    asides: [
      {
        id: 'editCompany',
        fields: {
          templateId: 'd43c4aec-8e34-4b5b-8f36-9266d4c5a484',
          companyId: '47436833-8656-496a-806e-3c48f8895815',
          providerId: '332aaa70-7881-4832-b7e6-3dd7a181041a',
          programId: '113f4ab3-dadf-4ee6-8ff5-e8e56ac72042',
          companyAccountId: 'd7b8f601-7901-4257-bf49-c81d5c19aebe',
 --->     status: 'Pending',
          managerEmail: 'kni@tradeshift.com'
        }
      }
    ]
  },

The field we are trying to update in the example is the status field in the asides array. Using the simplified example above you would expect the value to be updated using the props.onUpdate() function.

To validate the issue I changed the form to a simple div element and removed the data-ts attribute. Once completed, I attempted to set the value to an alternate option and clicked save; the value was indeed passed back to react and set properly for the selected row object.

RealDeanZhao commented 7 years ago

I think I was facing the similar issue for the data-ts: Form -> date type.. You may have to do in this way(I'm using redux-form):

import React from 'react';

export default class TSDate extends React.Component {
    componentDidMount() {
        const { input } = this.props;
        /**
         * The onchange event cannot be bind to the TS date input in this way: <input type='date' onChange={input.onChange} />
         * 
         * Unlike other form component of TS, the events of the date input have to be registerd here.  
         * This might be a bug when integrating TS date input with React
         */
        this.dateInput.name = input.name;
        this.dateInput.onblur = input.onBlur;
        this.dateInput.onchange = input.onChange;   // in your case, it should be props.onUpdate
        this.dateInput.ondragstart = input.onDragStart;
        this.dateInput.ondrop = input.onDrop;
        this.dateInput.onfocus = input.onFocus;
        this.dateInput.value = input.value;
    }
    render() {
        const { options } = this.props;

        return (
            <fieldset>
                <label>
                    <span>{options.label}</span>
                    <input type='date'
                        ref={(input) => { this.dateInput = input; }} />
                </label>
            </fieldset>
        )
    }
}
wiredearp commented 7 years ago

@tolrodco and @RealDeanZhao The design goal for UI components is to integrate with frameworks such as React in a completely transparent way so that developers can handle forms (events and validation and so on) in a completely standard way, so this is indeed a grave bug :cry:

wiredearp commented 7 years ago

@tolrodco (and @RealDeanZhao): Can you confirm that the bug is fixed in version 7.0.0-rc.7?

  1. Open config.json in the Apps-Server
  2. Change the runtime_version to 7.0.0-rc.7
  3. Restart the server
  4. Behold the component, now working as expected!
RealDeanZhao commented 7 years ago

@wiredearp Tried, not working for the date input.

sampi commented 7 years ago

@RealDeanZhao and @tolrodco can you give it another try with 7.0.0-rc.8 ? React was waiting for an input event for the DateInput, but a change event for the Select and we were sending change for both.

tolrodco commented 7 years ago

Yes, first thing I will do when I get into the office ... 

tolrodco commented 7 years ago

I switched to tsui7 for Apps-Server using 7.0.0-rc.8 and can confirm that the actions for select onChange worked as expected.

sampi commented 7 years ago

Excellent! I'm closing the issue and just keep an eye out for the releases page or watch/star the repo to get notified by email. The final release should be out tomorrow.

RealDeanZhao commented 7 years ago

@sampi , it works for 7.0.0-rc.8. Thank you.