-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Re-add the ability to return null in ListView.builder #108706
Conversation
(from triage): Looks like some tests are failing on this, could you take a look and fix those up? |
I'd happily try to fix the CI, but the error messages are unreadable Example:
Any thought on how I could get a readable explanation of the problem? Otherwise I'll have a hard time fixing the issue. |
The failure appears pretty clearly in the log [1]:
[1] https://cirrus-ci.com/task/6670346764943360?logs=main#L801 |
Thanks! Weird that Github action doesn't render them clearly. I'll look into it |
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.
Thanks for the updates. So we have updated PageView, ListView, and GridView here, and it looks like AnimatedList, ReorderableList are confirmed not relevant.
It also looks like CupertinoTabScaffold specifies in the docs that tabBuilder cannot be null.
For the three cases that are changing to NullableIndexedWidgetBuilder
can you add a templated doc that clarifies the behavior when using null to indicate the end of the list? The max scroll extent will not be accurate unless you are at the end of the list for example. This has caused confusion for a lot of folks in the past, so reintroducing this gives us a better chance to document it. :)
Sorry, I missed your message! I'll update the docs after the Flutter Vikings (1st of September). |
(triage): @rrousselGit Do you have another chance to look at this now that Vikings is over? |
Oops, sorry for the delay. Done |
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 LGTM with nits, but it looks like CI is unhappy. Can you rebase with master to see if it resolves the failures?
Co-authored-by: Kate Lovett <katelovett@google.com>
…utter into null-item-builder
Co-authored-by: Kate Lovett <katelovett@google.com>
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.
Thanks for the updates! The docs test looks unhappy, but I've noted where the issues are below.
/// list view's children are created in advance, or all at once when the | ||
/// [ListView] itself is created, it is more efficient to use the [ListView] | ||
/// constructor. Even more efficient, however, is to create the instances on | ||
/// demand using this constructor's `itemBuilder` callback. | ||
/// |
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 needs an /// {@endtemplate}
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.
There is one, it is in the paragraph above
This paragraph isn't part of the @template
Oh, my bad. Let me fix that |
…utter into null-item-builder
No worries, appeasing the CI gods can feel a bit tedious at times. Thanks for sticking with it! |
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.
auto label is removed for flutter/flutter, pr: 108706, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead. |
auto label is removed for flutter/flutter, pr: 108706, due to Validations Fail. |
Ah we need a second reviewer, I'll seek one. |
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.
LGTM 👍
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.
Changes are reviewed. LGTM.
Fixes a possible regression during NNBD migration that removed the ability to return
null
inListView.builder
'sitemBuilder
fixes #108705
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.