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

DataTable: All getXXXProps() functions always overwrite user supplied data #9534

Closed
1 task
hjgartner opened this issue Aug 24, 2021 · 3 comments
Closed
1 task

Comments

@hjgartner
Copy link

What package(s) are you using?

  • carbon-components
  • [x ] carbon-components-react v7.37.1

Detailed description

Describe in detail the issue you're having.
In the DataTable class, the usage pattern is to use the 'getXXXProps' functions to initialize the specific table entities. For example in using a radio selection table the table would look like this:

<TableBody>
	{rows.map(row => (
		<TableRow key={row.id}>
			<TableSelectRow {...getSelectionProps({
				row,
				checked: this.state.selectedId === row.id,
			})} />
              ........

Calling 'getSelectionProps()' is supposed to fill the returned object with the default values, and then add/overwrite the user specified values to the returned json object.
No matter what I specify as 'user' values they always get overwritten with the defaults.

Is this issue related to a specific component?
Yes:
https://github.com/carbon-design-system/carbon/blob/main/packages/react/src/components/DataTable/DataTable.js

What did you expect to happen? What happened instead? What would you like to
see changed?
I expect the values I set as 'user' values to be present after the function runs. The returned object always has the default values.

What browser are you working in?
Chrome, Firefox, Edge

What version of the Carbon Design System are you using?
v7.37.1

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?
IBM Business Automation Workflow

Additional information

The problem here is how the spread operator is being used in the 'getXXXProps' functions:
getHeaderProps
getExpandHeaderProps
getRowProps
getSelectionProps

If you look at 'getSelectionProps', for instance, it returns this:

  return {
    ...rest,
    checked: row.isSelected,
    onSelect: composeEventHandlers([
      this.handleOnSelectRow(row.id),
      onClick,
    ]),
    id: `${this.getTablePrefix()}__select-row-${row.id}`,
    name: `select-row-${row.id}`,
    ariaLabel: t(translationKey),
    disabled: row.disabled,
    radio: this.props.radio || null,
  };

The '...rest' contains all the user values passed in. It should be at the end of the json object, not at the beginning as anything declared further down will overwrite any of the values contained in '...rest'.

Seee ECMA-262 "ECMAScript® Language Specification", https://www.ecma-international.org/ecma-262/#sec-json.parse
See Note #2 from section 25.5.1.1 InternalizeJSONProperty ( holder, name, reviver ), which reads:

In the case where there are duplicate name Strings within an object, lexically preceding values for the same key shall be overwritten.

In other words, last-value-wins.

The code should look like this in all the getXXXProps functions, with '...rest' being the last property of the object:
return {
checked: row.isSelected,
onSelect: composeEventHandlers([
this.handleOnSelectRow(row.id),
onClick,
]),
id: ${this.getTablePrefix()}__select-row-${row.id},
name: select-row-${row.id},
ariaLabel: t(translationKey),
disabled: row.disabled,
radio: this.props.radio || null,
...rest
};

@hjgartner
Copy link
Author

Any updates on this - seems trivial to fix?

@tay1orjones
Copy link
Member

tay1orjones commented May 2, 2022

Hi @hjgartner, sorry for the delay in response. I can shed some light on what I think might be confusion around the intended usage more-so than buggy behavior.

Calling 'getSelectionProps()' is supposed to fill the returned object with the default values, and then add/overwrite the user specified values to the returned json object.

The first part is true, the second part isn't included in the intended scope of these prop getters. They provide the values derived from the internal state of the component, but don't allow you to override them. You can provide these as additional props after the prop getter though:

<TableSelectRow
  {...getSelectionProps({
    row,
    checked
  })}
  checked: this.state.selectedId === row.id,
/>;

If you're interested in managing this (and more) state yourself, you can avoid using DataTable altogether and instead use the individual table components to construct a fully-controlled table with your own custom state manager. Here's an example sandbox showing what this could look like.

Does this match the goal of your usage? If not, could you share more of how/why you're wanting to control the checked value of the rows and what the blockers might be using the above info?

@joshblack
Copy link
Contributor

Hi there! 👋 Going to close this out due to inactivity.

@joshblack joshblack closed this as not planned Won't fix, can't repro, duplicate, stale Jun 6, 2022
@joshblack joshblack moved this to ✅ Done in Design System Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants