-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Chore(ArrayField): add support for storeKey
to manage independent selection states
#10390
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import * as React from 'react'; | ||
import { render, screen, waitFor } from '@testing-library/react'; | ||
import { render, screen, waitFor, fireEvent } from '@testing-library/react'; | ||
import { | ||
CoreAdminContext, | ||
ResourceContextProvider, | ||
|
@@ -12,7 +12,23 @@ import { NumberField } from './NumberField'; | |
import { TextField } from './TextField'; | ||
import { Datagrid } from '../list'; | ||
import { SimpleList } from '../list'; | ||
import { ListContext } from './ArrayField.stories'; | ||
import { ListContext, TwoArrayFieldsSelection } from './ArrayField.stories'; | ||
|
||
beforeAll(() => { | ||
jest.spyOn(console, 'error').mockImplementation((message, ...args) => { | ||
if ( | ||
typeof message === 'string' && | ||
message.includes('React will try recreating this component tree') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a normal error message - it probably comes from your story, your test, or your implementation. |
||
) { | ||
return; | ||
} | ||
console.warn(message, ...args); // Still log other errors | ||
}); | ||
}); | ||
|
||
afterAll(() => { | ||
jest.restoreAllMocks(); | ||
}); | ||
|
||
describe('<ArrayField />', () => { | ||
const sort = { field: 'id', order: 'ASC' }; | ||
|
@@ -158,4 +174,41 @@ describe('<ArrayField />', () => { | |
).toBeTruthy(); | ||
}); | ||
}); | ||
|
||
it('should not select the same id in both ArrayFields when selected in one', async () => { | ||
render(<TwoArrayFieldsSelection />); | ||
await waitFor(() => { | ||
expect(screen.queryAllByRole('checkbox').length).toBeGreaterThan(2); | ||
}); | ||
|
||
const checkboxes = screen.queryAllByRole('checkbox'); | ||
|
||
expect(checkboxes.length).toBeGreaterThan(3); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By reading the test, I don't know why you wait for that. If you want to wait for the two lists to render, please use a |
||
|
||
// Select an item in the memberships list | ||
fireEvent.click(checkboxes[1]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you enabled rowClick, I suggest you trigger a click on a piece of text instead. This would make the test much more readable as it's not obvious what |
||
render(<TwoArrayFieldsSelection />); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You shouldn't need to rerender. If you want to come back to the initial state, use more than one |
||
|
||
await waitFor(() => { | ||
expect(checkboxes[1]).toBeChecked(); | ||
}); | ||
|
||
expect(checkboxes[3]).not.toBeChecked(); | ||
|
||
fireEvent.click(checkboxes[3]); // Portfolios row 1 | ||
render(<TwoArrayFieldsSelection />); | ||
|
||
await waitFor(() => { | ||
expect(checkboxes[3]).toBeChecked(); | ||
expect(checkboxes[1]).toBeChecked(); // Membership remains checked | ||
}); | ||
|
||
fireEvent.click(checkboxes[1]); | ||
render(<TwoArrayFieldsSelection />); | ||
|
||
await waitFor(() => { | ||
expect(checkboxes[1]).not.toBeChecked(); | ||
expect(checkboxes[3]).toBeChecked(); // Portfolios remain selected | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,3 +182,70 @@ export const InShowLayout = () => ( | |
</AdminContext> | ||
</TestMemoryRouter> | ||
); | ||
|
||
export const TwoArrayFieldsSelection = () => ( | ||
<TestMemoryRouter> | ||
<AdminContext> | ||
<ResourceContextProvider value="organizations"> | ||
<RecordContextProvider | ||
value={{ | ||
id: 1, | ||
name: 'Acme Corp', | ||
memberships: [ | ||
{ id: 1, userId: 1001, role: 'Admin' }, | ||
{ id: 2, userId: 1002, role: 'Member' }, | ||
], | ||
portfolios: [ | ||
{ | ||
id: 1, | ||
name: 'Growth Portfolio', | ||
creatorId: 1001, | ||
}, | ||
{ | ||
id: 2, | ||
name: 'Tech Innovations', | ||
creatorId: 1002, | ||
}, | ||
], | ||
}} | ||
> | ||
<Card sx={{ m: 1, p: 1 }}> | ||
<SimpleShowLayout> | ||
<TextField source="name" /> | ||
|
||
{/* Memberships ArrayField */} | ||
<ArrayField | ||
source="memberships" | ||
storeKey="organization_memberships" | ||
> | ||
<Datagrid | ||
rowClick="toggleSelection" | ||
bulkActionButtons={true} | ||
isRowSelectable={() => true} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rows are selectable by default, you shouldn't need this line. Same for the second ArrayInput. |
||
> | ||
<TextField source="id" /> | ||
<TextField source="role" /> | ||
</Datagrid> | ||
</ArrayField> | ||
|
||
{/* Portfolios ArrayField */} | ||
<ArrayField | ||
source="portfolios" | ||
storeKey="organization_portfolios" | ||
> | ||
<Datagrid | ||
rowClick="toggleSelection" | ||
bulkActionButtons={true} | ||
isRowSelectable={() => true} | ||
> | ||
<TextField source="id" /> | ||
<TextField source="name" /> | ||
</Datagrid> | ||
</ArrayField> | ||
</SimpleShowLayout> | ||
</Card> | ||
</RecordContextProvider> | ||
</ResourceContextProvider> | ||
</AdminContext> | ||
</TestMemoryRouter> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,9 +79,15 @@ const ArrayFieldImpl = < | |
>( | ||
props: ArrayFieldProps<RecordType> | ||
) => { | ||
const { children, resource, perPage, sort, filter } = props; | ||
const { children, resource, perPage, sort, filter, storeKey } = props; | ||
const data = useFieldValue(props) || emptyArray; | ||
const listContext = useList({ data, resource, perPage, sort, filter }); | ||
const listContext = useList({ | ||
data, | ||
resource: storeKey || resource, // Prioritize storeKey if provided | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer that you add support for a custom |
||
perPage, | ||
sort, | ||
filter, | ||
}); | ||
return ( | ||
<ListContextProvider value={listContext}> | ||
{children} | ||
|
@@ -99,6 +105,7 @@ export interface ArrayFieldProps< | |
perPage?: number; | ||
sort?: SortPayload; | ||
filter?: FilterPayload; | ||
storeKey?: string | false; | ||
} | ||
|
||
const emptyArray = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this is a footgun as it will hide potentially important errors. I'd prefer that you do the mock in individual tests and only when necessary