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

SelectPanel should compare Objects based on key or id instead of the Objects themselves #4315

Open
ajhenry opened this issue Feb 27, 2024 · 2 comments · May be fixed by #4324
Open

SelectPanel should compare Objects based on key or id instead of the Objects themselves #4315

ajhenry opened this issue Feb 27, 2024 · 2 comments · May be fixed by #4324
Labels
bug Something isn't working react

Comments

@ajhenry
Copy link
Contributor

ajhenry commented Feb 27, 2024

Description

When passing in objects to the SelectPanel component, the selected items may not render correctly since they are doing direct object comparison:

onAction: (itemFromAction, event) => {
item.onAction?.(itemFromAction, event)
if (event.defaultPrevented) {
return
}
if (isMultiSelectVariant(selected)) {
const otherSelectedItems = selected.filter(selectedItem => selectedItem !== item)
const newSelectedItems = selected.includes(item) ? otherSelectedItems : [...otherSelectedItems, item]
const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange']
multiSelectOnChange(newSelectedItems)
return
}
// single select
const singleSelectOnChange = onSelectedChange as SelectPanelSingleSelection['onSelectedChange']
singleSelectOnChange(item === selected ? undefined : item)
onClose('selection')
},

If the items are defined in the component, they could rerender and create new object references. Then this comparison check will always return false as the original objects are held in useState.

Instead, we should rely on comparing the key or id props as these are guaranteed* to be unique per object.

So our comparison function would instead look something like the following:

onAction: (itemFromAction, event) => {
          item.onAction?.(itemFromAction, event)

          if (event.defaultPrevented) {
            return
          }

          if (isMultiSelectVariant(selected)) {
            const otherSelectedItems = selected.filter(selectedItem => selectedItem.key !== item.key)
            const newSelectedItems = selected.find(selectedItem => selectedItem.key === item.key) ? otherSelectedItems : [...otherSelectedItems, item]

            const multiSelectOnChange = onSelectedChange as SelectPanelMultiSelection['onSelectedChange']
            multiSelectOnChange(newSelectedItems)
            return
          }

          // single select
          const singleSelectOnChange = onSelectedChange as SelectPanelSingleSelection['onSelectedChange']
          singleSelectOnChange(item.key === selected.key ? undefined : item)
          onClose('selection')
        },

cc: @ipc103

Steps to reproduce

Minimal Repo
https://codesandbox.io/p/sandbox/dreamy-cache-msfqyq

Version

v35.26.0

Browser

Chrome

@ajhenry ajhenry added the bug Something isn't working label Feb 27, 2024
@ajhenry
Copy link
Contributor Author

ajhenry commented Feb 27, 2024

@ipc103 and I are happy to take this on 😄

ipc103 added a commit to ipc103/primer-react that referenced this issue Feb 27, 2024
Towards primer#4315

The SelectPanel only did a basic equality check for the item state, meaning
that it depended on having the exact same objects on multiple pass throughs.

This isn't always possible as sometimes you may want to have different objects.

This is a first pass at adding support for using other checks for object
equality. Will still need to clean up the logic/code changes here.

Co-authored-by: Andrew Henry <ajhenry@github.com>
@ipc103
Copy link

ipc103 commented Feb 27, 2024

Still WIP, but we started working on a fix here: ipc103@f472da2 Next steps:

  • Refactor the duplicated logic
  • Add a couple of additional test cases
  • Maybe support other properties besides key as a fallback?

ipc103 added a commit to ipc103/primer-react that referenced this issue Feb 28, 2024
Towards primer#4315

The SelectPanel only did a basic equality check for the item state, meaning
that it depended on having the exact same objects on multiple pass throughs.

This isn't always possible as sometimes you may want to have different objects.

This replaces the equality check with a test for an `id` property on the object.

If the `id` property isn't present, we fallback to the old behavior.

Note that a previous version used the `key` prop, but we decided `id` was a
better interface. f472da2#r139163795
Co-authored-by: Andrew Henry <ajhenry@github.com>
@ipc103 ipc103 linked a pull request Feb 28, 2024 that will close this issue
13 tasks
ipc103 added a commit to ipc103/primer-react that referenced this issue Feb 28, 2024
Towards primer#4315

The SelectPanel only did a basic equality check for the item state, meaning
that it depended on having the exact same objects on multiple pass throughs.

This isn't always possible as sometimes you may want to have different objects.

This replaces the equality check with a test for an `id` property on the object.

If the `id` property isn't present, we fallback to the old behavior.

Note that a previous version used the `key` prop, but we decided `id` was a
better interface. f472da2#r139163795
Co-authored-by: Andrew Henry <ajhenry@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working react
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants