-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
[DNM] Remove ListIterator
#10880
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
base: master
Are you sure you want to change the base?
[DNM] Remove ListIterator
#10880
Conversation
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.
Does SimpleList
still works when you pass data
manually? It doesn't seem to be used and tested anymore.
Also, this is missing:
- Update
WithListContext
to accept optionalempty
,loading
anderror
components
packages/ra-core/src/controller/field/ReferenceManyFieldBase.stories.tsx
Outdated
Show resolved
Hide resolved
packages/ra-ui-materialui/src/list/SimpleList/SimpleList.stories.tsx
Outdated
Show resolved
Hide resolved
packages/ra-ui-materialui/src/list/SimpleList/SimpleList.stories.tsx
Outdated
Show resolved
Hide resolved
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.
A few issues related to WithListContext
.
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.
Nearly done!
Co-authored-by: Madeorsk <[email protected]>
280adcc
to
4b20bec
Compare
</ReferenceArrayFieldBase> | ||
</ListIterator> | ||
</ListBase> | ||
<ListBase |
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 code isn't better than the previous one. It underlines the fact that RecordsIterator should probably return null
while loading, without any warning.
|
||
export interface WithListContextProps<RecordType extends RaRecord> { | ||
if (isPending === true && loading) { |
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.
loading=null is a valid ReactNode and should be taken into account. I thing you should change the test to be as in https://github.com/marmelab/react-admin/pull/10864/files#diff-cdac9daf5ac7f12fe651a4357b777c27564f7372b0387256dd630f0a9d07c479
hasTertiaryText={!!tertiaryText} | ||
/> | ||
} | ||
empty={empty ?? <></>} |
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.
should be empty ?? null
IMO
render={({ data }) => ( | ||
<Root className={className} {...sanitizeListRestProps(rest)}> | ||
<RecordsIterator | ||
data={data} |
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 don't know why you need data here
{...sanitizeListRestProps(rest)} | ||
> | ||
<RecordsIterator | ||
data={data} |
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.
same
Problem
ListIterator
has a drawback:empty
,loading
anderror
states appear inside the wrapper (often a<ul>
). It makes it unusable in most cases.Solution
<SingleFieldList>
and<SimpleList>
to not use itWithListContext
to accept optionalempty
,loading
anderror
componentsAdditional Checks
master
for a bugfix or a documentation fix, ornext
for a feature[ ] The PR includes unit tests (if not possible, describe why)