Skip to content

Conversation

jcfilben
Copy link
Collaborator

@jcfilben jcfilben commented Oct 6, 2022

What does this PR do?

Fixes an issue in SelectMultiple where objects are not being correctly compared. This PR removes array function instances such as indexOf and includes which don't work well for arrays containing objects.

Note: marking as a draft, I still need to add jest tests

Where should the reviewer start?

See the getOptionIndex and arrayIncludes functions added in the util file.

What testing has been done on this PR?

Tested with existing storybook stories and the following story (see codesandbox to see before and after):

import React, { useState } from 'react';

import { SelectMultiple } from '../SelectMultiple';
const options = [
  { fieldType: 'SYSTEM', value: 'abc', display: 'ABC', other: 123 },
  { fieldType: 'SYSTEM', value: 'xyz', display: 'XYZ', other: 456 },
];
export const Test = () => {
  const [value, setValue] = useState([
    {
      fieldType: 'SYSTEM',
      value: 'abc',
      display: 'ABC',
      other: 123,
    },
  ]);
  return (
    <SelectMultiple
      showSelectedInline
      labelKey="display"
      valueKey="value"
      options={options}
      value={value}
      onChange={({ value }) => setValue(value)}
    />
  );
};

export default {
  title: 'Input/SelectMultiple/Test',
};

How should this be manually tested?

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Closes #6395

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards compatible

@jcfilben jcfilben marked this pull request as draft October 6, 2022 20:35
@jcfilben jcfilben marked this pull request as ready for review October 6, 2022 21:19
@jcfilben jcfilben requested a review from taysea October 6, 2022 21:20
@jcfilben jcfilben requested a review from taysea October 7, 2022 16:40
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good to me!

@ericsoderberghp ericsoderberghp merged commit cc1c9e2 into grommet:master Oct 7, 2022
@taysea
Copy link
Collaborator

taysea commented Oct 7, 2022

Related to #5198

@jcfilben jcfilben mentioned this pull request Oct 7, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectMultiple - pre selected object options not working
3 participants