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

Handling null value in Grid grouping cell. #53

Closed
simonssspirit opened this issue Aug 1, 2018 · 11 comments
Closed

Handling null value in Grid grouping cell. #53

simonssspirit opened this issue Aug 1, 2018 · 11 comments
Assignees
Labels
Discussion Technical discussion, questions, input needed Documentation Item which is related to a new article or update of an existing one in our documentation pkg:Grid

Comments

@simonssspirit
Copy link
Contributor

Currently, if the Grid has a null value inside the grouped column an error will be shown in the console as the null values do not have a toString method.

There are three approaches which we can take:

  1. Leave the current behavior and pre-process the data to convert the null values or to use a cell renderer to display the null values as desired by the developer.

  2. Automatically convert the null value to empty string when showing it inside the grouping row. The downside of this approach is that if there are actual values with an empty string, then two separate groups with the same grouping row will be shown requiring cell render to change the value of one of them.

  3. Show the null value as string null. The downside is that the null string is not a value that can be helpful to the end user thus requiring to overwrite it with a cell render.

All approaches may require cell renderer at the end, but some of the approaches will require it only in specific cases.

Please share your feedback and suggestion for another approach if it comes to mind.

cc/ @telerik/kendo-react-team

@simonssspirit simonssspirit added Bug Item which indicates that something is not working pkg:Grid Discussion Technical discussion, questions, input needed labels Aug 1, 2018
@rmaenk
Copy link

rmaenk commented Aug 1, 2018

Hello simonssspirit,

Maybe it would be simpler to add additional property for grouping descriptor. For instance (using typescript):
interface GroupDescriptor {
field: string;
nullValue?: string;
dir?: 'asc' | 'desc';
aggregates?: Array;

}
Then use "nullValue" inside grouping cell. If this property is not defined- just use empty string.
I guess this should allow developers to decide: how null value should be shown in grouping cell.
Regards Roman.

@rmaenk
Copy link

rmaenk commented Aug 1, 2018

Additionally, nullValue may be not only string. It may contain value with toString method (again typescript):
interface GroupDescriptor {
field: string;
nullValue?: string | { toString:()=>string; };
dir?: 'asc' | 'desc';
aggregates?: Array;

}
Roman

@Xizario
Copy link
Contributor

Xizario commented Aug 1, 2018

@Roamel We plan to detach the group?: GroupDescriptor[]; property from the actual rendering of the data.
See this related issue: #41
So you could be able to show grouped data without specifying the descriptors. and/or having descriptors in the header that are different from the actual grouping.

Since they will be detached, passing configuration to the descriptors does not mean it will be accessible/matched to the actual group header/footer. The group footers and headers on the other hand are direct representation of the group items in the data and could be used as configuration as well.

I think the more suitable solution is removing the problematic type casting.
{this.props.dataItem[this.props.field].toString()}
to become as simple as
{this.props.dataItem[this.props.field]}

Comes with the following benefits:

  • Solving the null to string issue.
  • Allows custom components to be rendered (like icons or buttons, practically anything) instead of just text, without need for custom group cell renders.
  • Does not require further configuration and will continue to work when we detach the rendering of the data from the configuration of the groups.

Update: this change can be tested here: https://stackblitz.com/edit/react-cthaqt?file=app%2Fmain.js

@rmaenk
Copy link

rmaenk commented Aug 1, 2018

Look good. However I am interesting about formatting this.props.dataItem[this.props.field]. How I can specify formatting if this.props.dataItem[this.props.field] is Date value?

@rmaenk
Copy link

rmaenk commented Aug 1, 2018

What about this way (isPrimitiveType, format and fieldFormat just a pseudo code):
var fieldValue = this.props.dataItem[this.props.field];//this is Date value, for instance isPrimitiveType(fieldValue) ? format(fieldValue, fieldFormat) : fieldValue

@rmaenk
Copy link

rmaenk commented Aug 1, 2018

Ah, it looks like, finally, I have got your idea:

    let processedData = process(products, dataState);
    //transform grouping dataitem value
    processedData.data = processedData.data.map(d => {
      return {
        ...d,
        value: <here can be format logic or jsx component , that should be possible by removing toString, as you mentioned above>
      }
    });

I like this approach
Thanks!

@Xizario
Copy link
Contributor

Xizario commented Aug 1, 2018

@Roamel , @telerik/kendo-react-team , @rkonstantinov any suggestions where the configuration for the format should be placed. I am opened for all kind of ideas.

If we check the type of the data and apply some predefined format culture and locale(as in your suggestion), you would need to be able to change the format. And here is the problem:

  • The Grid could render data that is grouped multiple times by the same field.
  • The data can be grouped by field, that is not in the columns.
  • Multiple columns bound to the same field could have different formats.
  • The data can be grouped by some more complex logic (outside the capabilities of the process method from the kendo-data-query packages)

Based on the above, we can't put configuration options for the group headers into the column declaration that the user defines. Because there is non one to one match between them.

An option is to add separate configuration for the formats, that to be passed down to the GroupCell, but it should be matched by something and that can't be the field, since there is no requirement for the DataResult to have unique field name in the group hierarchy.

Here is an example. Runnable version here https://stackblitz.com/edit/kendo-grid-sync-xktxqj?file=index.tsx

class App extends React.Component<{}, {}> {
  readonly state = {
    dataResult: {
      total: 2,
      data: [
        {
          value: 'This is the first group',
          items: [
            { a: new Date(), b: new Date(2010, 10, 10) },
            { a: new Date(), b: new Date(2010, 10, 10) }]
        },
        {
          value: 55, //This is the second group, but have different type of the value
          items:
          [{ a: null, b: 'yet another type' }]
        },
        {
          value: new Date(), // The third group
          items: [
            { a: new Date(), b: 1234 },
            { a: new Date(), b: null }]
        }
      ]
    }
  }

  render() {
    return (
      <Grid
        data={this.state.dataResult}
        group={[{ field: '' }]} // you would be able to remove that when #41 got fixed
        reorderable={true}
      >
        <GridColumn field="a" format="{0:dd MM yyyy}" />
        <GridColumn title="some column without type" field="b" />
        <GridColumn title="a with different format" field="a" format="{0:yy-MM-dd-hh-mm-ss}" />
      </Grid>);
  }
}

I think the above illustrates the problem of specifying any format for the groups. So if we need to keep the grid flexible for showing different kind of data without restrictions, it is best if we treat the group data items value as "Label" rather than "Value". It is easy for the user to decide if he wants to format them before passing down to the grid. (more over he could already do that)

If the issue is with the process or groupBy from our @progress/kendo-data-query (the packages is generic and used in Angular as well) we could probably ship an extension methods particularity for React that gets configuration of formats and apply them to the processed data and utilize the IntlProvider for the formats as well. What do you think?

@rmaenk
Copy link

rmaenk commented Aug 1, 2018

Removing toString in GridGroupCell is fine for me. process is not an issue in our case. We can workaround null issue with additional processing for each group data item value using recursive function. At the same time we can apply formatting for it, something like this:

    private getData(dataSource, gridState) {
            const result = process(dataSource, gridState);
            transformGroupItem(result.data);
            return result;
    }
    private transformGroupItem(items: any[]) {
        items.forEach((item) => {
            const groupItem = item as GridGroupRow;
            if (typeof groupItem.field !== "undefined") {
                if (typeof groupItem.items !== "undefined") {
                    this.transformGroupItem(groupItem.items);
                }
                groupItem.value = formatGroupValue(groupItem.field, groupItem.value);
            }
        });
    }

function formatGroupValue(groupField: string, groupValue: any){
    switch(groupField){
        case "startDate":
              return groupValue != null ? format(groupValue, "d"): "Start date not specfied";
        case "cost":
              return groupValue != null ? format(groupValue, "c2"): "Free";
        default:
              return groupValue != null ? groupValue.toString() : "Not set";
    }
}

Additionally we want to reuse format for group item values and grid columns.

If you able to provide extension method that doing similar logic as above it would be helpful.
Note: this discussion is related to our support ticket: 1179448
Regards,
Roman

@Xizario
Copy link
Contributor

Xizario commented Oct 18, 2018

Since it does not throw error I am removing the bug item now, some documentation need to be improved for custom formatting and then we can close this

@Xizario Xizario added Documentation Item which is related to a new article or update of an existing one in our documentation and removed Bug Item which indicates that something is not working labels Oct 18, 2018
@simonssspirit
Copy link
Contributor Author

This is another approach to change the value, using cellRender and modifying the children prop.

https://stackblitz.com/edit/react-hdeimf?file=app/main.js

@simonssspirit
Copy link
Contributor Author

This is documented: https://www.telerik.com/kendo-react-ui/knowledge-base/render-a-custom-content-inside-the-grid-group-header/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Technical discussion, questions, input needed Documentation Item which is related to a new article or update of an existing one in our documentation pkg:Grid
Projects
None yet
Development

No branches or pull requests

3 participants