Repository navigation
Fix SearchAnchor.bar to respect SearchBarThemeData padding - #186037
CoderBoxer wants to merge 3 commits into
Conversation
github-actions
Bot
added
framework
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates SearchAnchor.bar to respect SearchBarThemeData.padding and adds a regression test. The review feedback suggests refactoring the test to verify the padding property on the SearchBar widget directly, which improves test robustness by avoiding reliance on internal implementation details.
| final Padding paddingWidget = tester.widget<Padding>( | ||
| find.descendant(of: find.byType(SearchBar), matching: find.byType(Padding)).first, | ||
| ); | ||
|
|
||
| expect(paddingWidget.padding, customPadding); |
There was a problem hiding this comment.
Instead of searching for a descendant Padding widget, which is an implementation detail of SearchBar, it is more robust to verify the padding property on the SearchBar widget itself. This ensures the test is directly checking the property passed to the widget rather than relying on the internal widget tree structure.
| final Padding paddingWidget = tester.widget<Padding>( | |
| find.descendant(of: find.byType(SearchBar), matching: find.byType(Padding)).first, | |
| ); | |
| expect(paddingWidget.padding, customPadding); | |
| final SearchBar searchBar = tester.widget<SearchBar>(find.byType(SearchBar)); | |
| expect(searchBar.padding?.resolve(<WidgetState>{}), customPadding); |
References
- Assess whether the code can be made simpler or refactored to enhance readability and maintainability. (link)
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
LGTM! Thanks for your contribution!
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM. Thank you for finding it!
|
There's analyzer issues. Can you address it? |
Hi @dkwingsmt, I merged the latest upstream/master into the branch and ran the analyzer locally. It returned 'No issues found!', so the branch is synced and pushed. Let me know if the CI still complains! |
|
Hey @dkwingsmt , the CI failed on the internal 'Flutter Roll on Borg' step, and I don't have access to view those internal logs. Could you take a quick look to see what broke when you have a moment? |
|
@CoderBoxer I'll take care of the merging process and I'll let you know if anything's worth your notice. Thank! |
|
Thanks for your contribution! This PR has reached completion, however we're not able to merge this PR for the moment due to the Material/Cupertino code freeze (#184093). As soon as the decoupling is complete, we will post a way to port this PR to the new packages to land. In the meantime, I'll convert this PR to a draft and apply a label so that we can quickly find it when the freeze is lifted. |
dkwingsmt
added
the
waiting for code freeze
Piinks
added
the
Decoupling: Not ready to port yet
|
I've marked this PR as not ready to port to flutter/packages yet. |
|
Understood, thanks for the heads up @Piinks I'll leave this as-is and wait for the porting instructions once the decoupling is complete |
|
If you would like to return to this, material_ui and cupertino_ui are now accepting contributions. :) |
dkwingsmt
removed
the
Decoupling: Not ready to port yet
dkwingsmt
added
the
Decoupling: Port to flutter/packages
#13360) Fixes flutter/flutter#185889 Ports flutter/flutter#186037. Part of flutter/flutter#188444. Makes `SearchAnchor.bar` use `SearchBarThemeData.padding` when `barPadding` is null. cc @CoderBoxer I used AI assistance to port the approved fix. ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
Description
This PR fixes an issue where
SearchAnchor.barwas bypassingSearchBarThemeData.paddingand falling back directly to a hardcoded default of16.0.I added
SearchBarTheme.of(context).paddingto the resolution chain in_SearchAnchorWithSearchBarso that it correctly inherits the global theme properties before falling back to the defaults.(Note:
barLeadingretains its existing fallback behavior sinceSearchBarThemeDatadoes not possess aleadingproperty).Related Issue
Fixes #185889
Tests
search_anchor_test.dartto verify thatSearchAnchor.barproperly respects theSearchBarThemeData.paddingproperty.