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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bugs with custom field types and display bug with dropdown in IE #136

Merged
merged 7 commits into from Jan 19, 2019

Conversation

Projects
None yet
2 participants
@spplante
Copy link
Contributor

spplante commented Dec 29, 2018

Q A
Bug fix? [x]
New feature? [ ]
New sample? [ ]
Related issues? #135

What's in this Pull Request?

Fixed 3 different issues :

First issue

The first issue occurs when you have a custom field and you remove a row in the middle of multiple rows. All of a sudden the row following the deleted row (the one that was right after it) gets its custom field value replaced by the one that was set on the row that just got deleted. Note that this problem does not happen with the provided example because it isn't a controlled component (which uses the state to set the selected item for example), but this issue will occur with any custom field rendering a controlled component.

This is because as explained here , assigning keys to components using an index can cause lots of problems and this is exactly what this snippet from the CollectionDataViewser.tsx does :

{
    (this.state.crntItems && this.state.crntItems.length > 0) && (
      this.state.crntItems.map((item, idx, allItems) => (
                <CollectionDataItem key={idx}
                                    fields={this.props.fields}
                                    index={idx}
                                    item={item}
                                    [...] />
      ))
    )
}

This causes React to get mixed up in the different states later on. So in order to correctly create a unique but permanent key for the CollectionDataItem components, I modified it's constructor in order to add a uniqueId property that gets filled with a unique GUID when the "dummy/empty" row gets initialized :

constructor(props: ICollectionDataItemProps) {
    super(props);

    // Create an empty item with all properties
    let emptyItem = this.generateEmptyItem();
    
    [...]
}

  
private generateEmptyItem(): any {
    // Create an empty item with all properties
    let emptyItem:any = {};
    emptyItem.uniqueId = uuid();
    
    for (const field of this.props.fields) {
        // Assign default value or null to the emptyItem
        emptyItem[field.id] = field.defaultValue || null;
    }
    return emptyItem;
}

I also modified the addRow method in order to create a new uniqueId whenever we insert the current row and need to reset the "dummy/empty" row :

private addRow = () => {
    if (this.props.fAddItem) {
        [...]

        // Clear all field values
        let emptyItem = this.generateEmptyItem();
        this.setState({
          crntItem: {...emptyItem}
        });
      }
    }
  }

This allows the CollectionDataViewer.tsx class to use this later on when it's time to specify keys for the components :

{
            (this.state.crntItems && this.state.crntItems.length > 0) && (
              this.state.crntItems.map((item, idx, allItems) => (
                <CollectionDataItem key={item.uniqueId}
                                    fields={this.props.fields}
                                    index={idx}
                                    item={item}
                                    [...] />
              ))
            )
          }

This way each row/component will always have its own uniqueId/key from the moment it's created, and that uniqueId/key will remain the same no matter how you sort/move/delete rows, preventing React to get confused.

Second issue

As described in issue #135, there was a bug where the dummy "empty row" (which needs to be reseted after a new row insertion) would keep its value for custom field types that are rendering a controlled component. The code which is normally responsible for clearing all the field values in order to "reset" the "dummy/empty" row is the following :

private addRow = () => {
    if (this.props.fAddItem) {
        [...]

        // Clear all field values
        let emptyItem = this.generateEmptyItem();
        this.setState({
          crntItem: {...emptyItem}
        });
      }
    }
  }

However, this is not a code issue, but most likely caused by the fact that a controlled component rendered in the onCustomRender() function uses its state to define its value, which means even though we try to rerender the component with a null value property, the component will ignore the new prop and use its state value instead.

So in order for the "dummy/empty" row to update it's custom field's component state (when fields need to be cleared), I needed a way to tell the onCustomRender() function that we are now dealing with a brand new row which needs the component to rerender from scratch. For this I simply updated the ICustomCollectionField.ts interface and modified the onCustomRender() signature with the following :

onCustomRender?: (field: ICustomCollectionField, value: any, onUpdate: (fieldId: string, value: any) => void, rowUniqueId: string) => JSX.Element;

By adding the row/item uniqueId (created to fix issue #1) to the function's signature, the user can now modify the componentDidUpdate of his controlled component and specify his own logic that will rerender his component from scratch :

public componentDidUpdate(prevProps: IAsyncDropdownProps, prevState: IAsyncDropdownState): void {
    if (this.props.rowUniqueId !== prevProps.rowUniqueId) {
        this.setState({
            selectedKey: null
        });
    }
}

That way when the "dummy/empty" row needs to be reset and have its field values cleared, its uniqueId will get regenerated (as shown in fix #1), the onCustomRender function will be called with the newly generated row's unique Id specified in the rowUniqueId parameter, and from that point it is up to the user to make sure the component rerenders if the uniqueId is different from the last one 馃憤

Third issue

The last issue was a simply display bug occurring with dropdown fields on IE (and on Chrome when the dropdown had no options). The alignment would get all mixed and I added a simple line in the CSS to fix it.

@spplante spplante referenced this pull request Dec 29, 2018

Closed

PropertyFieldCollectionData: Bug with onCustomRender() #135

1 of 3 tasks complete

spplante added some commits Dec 29, 2018

Fixed a display bug with dropdowns in IE
Fixed a display bug with dropdowns appearing all messed up when using IE (or in Chrome when dropdowns have to options).
Fixed a state bug because of keys generation
Fixed a state bug because of the way the keys were generated for the CollectionDataItem components. They were generated based on the index which causes all sorts of problems.
Fixed a state bug because of keys generation
Fixed a state bug because of the way the keys were generated for the CollectionDataItem components. They were generated based on the index which causes all sorts of problems.

@spplante spplante changed the title Fixed a bug where new rows would get same value as previous row for custom field types Fixed bugs with custom field types and display bug with dropdown in IE Dec 30, 2018

@estruyf estruyf merged commit b8a99ec into SharePoint:dev Jan 19, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla All CLA requirements met.
Details

@estruyf estruyf added this to the 1.14.0 milestone Jan 19, 2019

@estruyf

This comment has been minimized.

Copy link
Collaborator

estruyf commented Jan 19, 2019

@spplante thank you very much for these fixes! Merged them and they will be available in 1.14.0.

@estruyf estruyf referenced this pull request Jan 24, 2019

Merged

Merge for 1.14.0 #148

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