-
-
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?
Conversation
storeKey
to manage independent selection states**
storeKey
to manage independent selection states**storeKey
to manage independent selection states
@fzaninotto Can you review? thanks |
Thanks for the PR! As this is a new feature, can you target the |
Yes, I can. Thanks |
d8c1f4f
to
d852f0c
Compare
@djhi @fzaninotto I have updated the target branch as well as written the necessary stories and tests for the feature. Please re-review. Thanks |
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.
Too bad you took a wrong direction with the fix, but we appreciate the effort to provide good stories, tests, and documentation! Thanks!
useEffect(() => { | ||
if (!resource) return; | ||
// Check if storeKey exists in the pagination store | ||
const currentPagination = storeKeyPaginationRef.current[resource]; | ||
if (currentPagination) { | ||
// Restore existing pagination state for the storeKey | ||
if ( | ||
page !== currentPagination.page || | ||
perPage !== currentPagination.perPage | ||
) { | ||
setPage(currentPagination.page); | ||
setPerPage(currentPagination.perPage); | ||
} | ||
} else { | ||
setPage(initialPage); | ||
setPerPage(initialPerPage); | ||
} | ||
storeKeyPaginationRef.current[resource] = { page, perPage }; | ||
}, [ | ||
resource, | ||
setPage, | ||
setPerPage, | ||
initialPage, | ||
initialPerPage, | ||
page, | ||
perPage, | ||
]); | ||
|
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.
I'm afraid you are going in the wrong direction here 😬 . The fix for #10382 suggested by @fzaninotto was to allow to override the storeKey
used to store the selection state (i.e. used in useRecordSelection
), but not the pagination state.
You can have a look at what was done in useReferenceManyFieldController
for an example:
react-admin/packages/ra-core/src/controller/field/useReferenceManyFieldController.ts
Lines 90 to 92 in 7b319c4
const [selectedIds, selectionModifiers] = useRecordSelection({ | |
resource: storeKey, | |
}); |
export const ListsWithoutStoreKeys = () => ( | ||
<CoreAdminContext store={localStorageStore()} dataProvider={dataProvider}> | ||
<CoreAdminUI layout={Layout}> | ||
<CustomRoutes> | ||
<Route path="/store" element={StorePosts} /> | ||
<Route path="/nostore" element={NoStorePosts} /> | ||
</CustomRoutes> | ||
<Resource name="posts" /> | ||
</CoreAdminUI> | ||
</CoreAdminContext> | ||
); |
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.
This Story is a bit misleading, StorePosts
is still using a storeKey
.
Besides, storeKey={false}
is not the same as having no storeKey
(for NoStorePosts
). I'd expect both lists in ListsWithoutStoreKeys
to have no storeKey
.
const Layout = (props: CoreLayoutProps) => ( | ||
<div style={styles.mainContainer}> | ||
<Link aria-label="top" to={`/top`}> | ||
Go to Top Posts | ||
</Link> | ||
<Link aria-label="flop" to={`/flop`}> | ||
Go to Flop Posts | ||
</Link> | ||
<Link aria-label="store" to={`/store`}> | ||
Go to Store List | ||
</Link> | ||
<Link aria-label="nostore" to={`/nostore`}> | ||
Go to No-Store List | ||
</Link> | ||
|
||
<br /> | ||
{props.children} | ||
</div> | ||
); |
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.
Layout
should not be shared across stories since both stories do not implement the same routes
docs/ArrayField.md
Outdated
|
||
When displaying multiple lists with the same data source, you may need to distinguish their selection states. To achieve this, assign a unique `storeKey` to each `ArrayField`. This allows each list to maintain its own selection state independently. | ||
|
||
In the example below, two `ArrayField` components display the same data source (`books`), but each stores its selection state under a different key (`books.selectedIds` and `custom.selectedIds`). This ensures that both components can coexist on the same page without interfering with each other's state. |
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.
In the example below, two `ArrayField` components display the same data source (`books`), but each stores its selection state under a different key (`books.selectedIds` and `custom.selectedIds`). This ensures that both components can coexist on the same page without interfering with each other's state. | |
In the example below, two `ArrayField` components display the same data source (`books`), but each stores its selection state under a different key (`customOne.selectedIds` and `customTwo.selectedIds`). This ensures that both components can coexist on the same page without interfering with each other's state. |
d852f0c
to
46a45d8
Compare
4b92f80
to
924b2d5
Compare
924b2d5
to
3a259c9
Compare
3a259c9
to
dc04998
Compare
dc04998
to
89d445b
Compare
@slax57 Can you re-review? Thanks. |
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 comment
The 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.
import { ListContext } from './ArrayField.stories'; | ||
import { ListContext, TwoArrayFieldsSelection } from './ArrayField.stories'; | ||
|
||
beforeAll(() => { |
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
<Datagrid | ||
rowClick="toggleSelection" | ||
bulkActionButtons={true} | ||
isRowSelectable={() => true} |
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.
Rows are selectable by default, you shouldn't need this line. Same for the second ArrayInput.
|
||
const checkboxes = screen.queryAllByRole('checkbox'); | ||
|
||
expect(checkboxes.length).toBeGreaterThan(3); |
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.
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 findByText
looking for the actual array values instead.
expect(checkboxes.length).toBeGreaterThan(3); | ||
|
||
// Select an item in the memberships list | ||
fireEvent.click(checkboxes[1]); |
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.
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 checkboxes[1]
is.
|
||
// Select an item in the memberships list | ||
fireEvent.click(checkboxes[1]); | ||
render(<TwoArrayFieldsSelection />); |
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.
You shouldn't need to rerender. If you want to come back to the initial state, use more than one it()
and group them in a describe()
.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that you add support for a custom storeKey
in useList
. I know that we currently use resource
only for selection in useList
, but this may change in the future.
Problem
#10382
Describe the problem this PR solves
Solution
storeKey
property forArrayField
to enable distinct selection state storage for multiple instances of the same data source.https://www.loom.com/share/b6da1650760d4a83981f602341b667e7?sid=3fa2812e-1cdc-4266-9958-89c9ff731657
Describe the solution this PR implements
How To Test
Describe the steps required to test the changes
Additional Checks
master
for a bugfix, ornext
for a featureAlso, please make sure to read the contributing guidelines.