-
Notifications
You must be signed in to change notification settings - Fork 3.4k
docs: add migration guide for findChildIndexCallback of separated builder #12636
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: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @rkishan516, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces essential documentation for a breaking change in Flutter's list widgets. It provides a clear migration guide for developers transitioning from the deprecated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This PR adds a helpful migration guide for the findChildIndexCallback deprecation. The explanation is clear and the code examples are useful. I've found a few issues to address:
- There's a typo in the new markdown file's name (
seperatedshould beseparated). This typo is also present in the link withinindex.mdand the PR title. - The "after migration" code examples for both
ListView.separatedandSliverList.separatedare incorrect. They should returnnullwhen an item is not found, but they currently return-1. - There's a missing link definition for
ChildIndexGetterin the new migration guide, which will cause a broken link.
Please see my detailed comments for suggestions on how to fix these.
| [Stop generating `AssetManifest.json`]: /release/breaking-changes/asset-manifest-dot-json | ||
| [`$FLUTTER_ROOT/version` replaced by `$FLUTTER_ROOT/bin/cache/flutter.version.json`]: /release/breaking-changes/flutter-root-version-file | ||
| [Deprecate `TextField.canRequestFocus`]: /release/breaking-changes/can-request-focus | ||
| [Deprecated findChildIndexCallback for separated constructors]: /release/breaking-changes/seperated-builder-find-child-index-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.
There's a typo in this URL path: seperated should be separated. This typo also exists in the new file's name, which will need to be corrected as well.
| [Deprecated findChildIndexCallback for separated constructors]: /release/breaking-changes/seperated-builder-find-child-index-callback | |
| [Deprecated findChildIndexCallback for separated constructors]: /release/breaking-changes/separated-builder-find-child-index-callback |
| final ValueKey<String> valueKey = key as ValueKey<String>; | ||
| // Return item index directly - no need to multiply by 2 | ||
| return items.indexOf(valueKey.value); |
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.
The implementation of findItemIndexCallback is incorrect. The callback should return null if the key is not found, but List.indexOf returns -1 in that case. The current implementation will return -1, which is not the expected behavior. You should check for -1 and return null instead.
| final ValueKey<String> valueKey = key as ValueKey<String>; | |
| // Return item index directly - no need to multiply by 2 | |
| return items.indexOf(valueKey.value); | |
| final ValueKey<String> valueKey = key as ValueKey<String>; | |
| final int itemIndex = items.indexOf(valueKey.value); | |
| return itemIndex == -1 ? null : itemIndex; |
| final ValueKey<String> valueKey = key as ValueKey<String>; | ||
| return items.indexOf(valueKey.value); |
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.
Similar to the ListView.separated example, this implementation is incorrect. It should return null when the item is not found, not -1. Please update the code to handle the not-found case correctly.
| final ValueKey<String> valueKey = key as ValueKey<String>; | |
| return items.indexOf(valueKey.value); | |
| final int itemIndex = items.indexOf((key as ValueKey<String>).value); | |
| return itemIndex == -1 ? null : itemIndex; |
|
|
||
| * [`ListView.separated`][] | ||
| * [`SliverList.separated`][] | ||
| * [`ChildIndexGetter`][] |
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.
994594c to
9517b90
Compare
|
Visit the preview URL for this PR (updated for commit 9517b90): https://flutter-docs-prod--pr12636-seperated-builder-child-cal-zhe9wd2v.web.app |
chunhtai
left a comment
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.
migration guide lgtm except for the gemini comment
sfshaza2
left a comment
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, but @Piinks, can you either review or delegate review of this PR? Also, the bot has some feedback that needs to be addressed.
Looks like @chunhtai is already providing review? |
|
@rkishan516, as @chunhtai has mentioned. Can you fix the gemini issues and then I can land. |
| * [`$FLUTTER_ROOT/version` replaced by `$FLUTTER_ROOT/bin/cache/flutter.version.json`][] | ||
| * [Stop generating `AssetManifest.json`][] | ||
| * [Deprecate `TextField.canRequestFocus`][] | ||
| * [Deprecated findChildIndexCallback for separated constructors][] |
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.
Separated constructors here seems vague. It's not clear to me what API this line is referring to.
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.
What about something like:
[Deprecate `findChildIndexCallback` in favor of `findItemIndexCallback` in `ListView` and `SliverList` separated constructors][]
Or is there a character limit?
…ated named constructor in ListView and SliverList (#174491) FindChildIndexCallback to take seperators into account for seperated named constructor in ListView and SliverList fixes: #174261 ## Migration guide flutter/website#12636 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
@sfshaza2 I have pushed all required changes. |
This PR add breaking changes migration guide for findChildIndexCallback of seperated builders in ListView and SliverList.
Presubmit checklist
of 80 characters or fewer.