nulogy / design-system

Nulogy Design System
http://nulogy.design
MIT License
64 stars 13 forks source link

The ColumnType has dataKey masked optional and it's used as `key` for columns #1292

Closed danielsilva closed 8 months ago

danielsilva commented 8 months ago

Describe the bug

When no dataKey is passed a couple of warnings show up in the browser console:

image

image

This requires fake dataKey values to be passed in for columns that do not have a specific data key to read its values from. One example is the actions columns that some tables can have, but another example is a column that implements a custom cellFormatter and displays a combination of values from multiple fields (e.g. a column that displays actual/expected quantities)

The workaround of using fake datakey values prevents us from being able to assign Typescript constraints to the Table columns like the following:

type JobsTableColumn = Omit<ColumnType, "dataKey"> & { dataKey?: keyof Job };

Steps to reproduce

function Table() {
  const columns = [
    { label: "Date", dataKey: "date" },
    { label: "Actual/Expected Quantity", cellFormatter: QuantityCell },
  ];

  const rowData = [
    {
      date: "2019-10-01",
      expectedQuantity: "2,025 eaches",
      actualQuantity: "1,800 eaches",
      id: "r1",
    },
    {
      date: "2019-10-02",
      expectedQuantity: "2,475 eaches",
      actualQuantity: "2,250 eaches",
      id: "r2",
    },
  ];

  return (
    <Table
      columns={columns}
      rows={rowData}
      rowsPerPage={number("Rows per page", 1)}
      onPageChange={action("page changed")}
      className="Table"
    />
  );
}

function QuantityCell({ row }) {
  const { actualQuantity, expectedQuantity } = row;
  const uom = actualQuantity.value ? actualQuantity.uom.shortLabel : "";

  return (
    <Text>
      {expectedQuantity.value ?? "--"} / {actualQuantity.value ?? "--"} {uom}
    </Text>
  );
}

Expected behaviour

No warnings are displayed in the browser console regarding empty key

Is your team blocked from moving forward by the bug?

No

Who would you like to fix the bug?

Helpful resources

No response

Additional context

No response

haideralsh commented 8 months ago

Hey @danielsilva, thanks for opening this.

I was looking at the different options we have here, I think we could add a prop key that can be used to specify the key for columns that don't use the dataKey. I am a little hesitant to use the index as a fallback incase an application has an option to let the user show and hide columns.

What do you think?

github-actions[bot] commented 8 months ago

:tada: This issue has been resolved in version 8.15.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: