-
-
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
[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
WithListContextto accept optionalempty,loadinganderrorcomponents
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 <matthieu@marmelab.com>
280adcc to
4b20bec
Compare
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.
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
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
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
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
ListIterator (削除ここまで)ListIterator (追記ここまで)
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.
Do not remove it: mark it as deprecated instead.
Problem
ListIteratorhas a drawback:empty,loadinganderrorstates appear inside the wrapper (often a<ul>). It makes it unusable in most cases.Solution
<SingleFieldList>and<SimpleList>to not use itWithListContextto accept optionalempty,loadinganderrorcomponentsAdditional Checks
masterfor a bugfix or a documentation fix, ornextfor a feature(削除) [ ] The PR includes unit tests (if not possible, describe why) (削除ここまで)