Repository navigation
[material_ui] Add supportingTextPadding to InputDecorator - #12769
MohanadAbdallah-mv wants to merge 5 commits into
Conversation
github-actions
Bot
added
p: material_ui
triage-design
![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 introduces the supportingTextPadding property to InputDecorator, InputDecoration, InputDecorationTheme, and InputDecorationThemeData to allow custom padding for the supporting text row, while renaming internal 'subtext' references to 'supportingText'. Feedback on these changes highlights that InputDecorationThemeData.lerp is missing interpolation for the new supportingTextPadding property, which is necessary for smooth theme transitions, and suggests adding a corresponding test case to verify this behavior.
| border: border ?? other.border, | ||
| constraints: constraints ?? other.constraints, | ||
| visualDensity: visualDensity ?? other.visualDensity, | ||
| supportingTextPadding: supportingTextPadding ?? other.supportingTextPadding, | ||
| ); |
There was a problem hiding this comment.
The InputDecorationThemeData.lerp method is missing the interpolation for supportingTextPadding. Without this, transitions between different input decoration themes will not animate the supporting text padding smoothly.
Please update InputDecorationThemeData.lerp to include:
supportingTextPadding: EdgeInsetsGeometry.lerp(a?.supportingTextPadding, b?.supportingTextPadding, t),| testWidgets('supportingTextPadding defined in InputDecorationTheme is used for supporting text', ( | ||
| WidgetTester tester, | ||
| ) async { | ||
| const themePaddingStart = 40.0; | ||
| const themePaddingEnd = 20.0; |
There was a problem hiding this comment.
Consider adding a test case to verify that InputDecorationThemeData.lerp correctly interpolates supportingTextPadding between two themes. This ensures that theme animations work as expected and prevents future regressions.
|
@dkwingsmt @Renzo-Olivares |
|
Thanks for the contribution! You haven’t completed all of the items in the PR checklist above, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when that’s complete please feel free to mark the PR as ready for review. |
b91ef36 to
a25c5f2
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces the supportingTextPadding property to InputDecorator, InputDecoration, InputDecorationTheme, and InputDecorationThemeData to allow custom padding for the supporting text row, alongside renaming internal subtext-related members to use 'supportingText'. Feedback suggests adding the new property to InputDecorationThemeData.lerp to ensure correct interpolation during theme transitions.
| this.alignLabelWithHint = false, | ||
| this.constraints, | ||
| this.visualDensity, | ||
| this.supportingTextPadding, |
There was a problem hiding this comment.
The new supportingTextPadding property should be added to InputDecorationThemeData.lerp to ensure that it is correctly interpolated during theme transitions (e.g., using EdgeInsetsGeometry.lerp).
…ation (flutter#183582) **Maintainer's note:** This PR was originally named "Adding supportingTextPadding to InputDecorator and InputDecorationTheme", and has been split into two parts due to decoupling. The majority of the change is now applied by flutter/packages#12769 . This PR only addresses the terminology change, since we discovered that the M3 spec (https://m3.material.io/components/text-fields/specs ) officially refers to error text and hint text as "supporting text". ----- This PR introduces the `supportingTextPadding` property to `InputDecorator`, `InputDecoration`, and `InputDecorationThemeData`. While originally conceived as `errorPadding`, this property was renamed to `supportingTextPadding` during review to align with the Material Design 3 specification, as it controls the padding for the entire supporting text row (which includes helper text, error text, and counter widgets). Additionally, this PR addresses layout, intrinsic sizing, and code consistency concerns raised during the review process: #### Key Features & Refactors: 1. **Added `supportingTextPadding`:** Allows complete, independent control over the padding of the supporting text row (helper/error/counter). 2. **Decoupled Layout Constraints:** Introduced `supportingTextConstraints` in `_layout()` to bound the supporting text row independently of `contentConstraints`, ensuring custom paddings do not break or shift the layout of inline `prefix`/`suffix` widgets. 3. **Fixed Intrinsic Height Mismatches:** Corrected `computeMinIntrinsicHeight()` to compute `supportingTextWidth` separately from the primary width deflation path, ensuring intrinsic height calculations accurately predict wrapped text heights. 4. **Unified Terminology:** Renamed all legacy internal `subtext` references (e.g. `_SubtextSize`, `_computeSubtextSizes`, `subtextHeight`) to `supportingText` to match the public property name and the M3 spec. *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* Fixes flutter#175834 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [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]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Tong Mu <[email protected]>
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Also I suggest adding the following test cases
- An
icontogether withsupportingTextPadding. The_boxSize(icon).widthoffset changed in both directions and isn't covered. - M2 behavior (the 8.0 gap).
Otherwise LGTM. Thank you!
Co-authored-by: Tong Mu <[email protected]>
|
|
||
| /// Overrides the default value for [InputDecoration.supportingTextPadding]. | ||
| /// | ||
| /// This property is obsolete and will be deprecated in a future release: |
There was a problem hiding this comment.
If so, can we only add supportingTextPadding to InputDecorationThemeData?
There was a problem hiding this comment.
supportingTextPadding exists for both InputDecorationThemeData and InputDecorationTheme, although the InputDecorationTheme will be deprecated in a future release, what is the correct approach towards this concern? Do we still apply the change to it till it gets deprecated or we ignore it completely and only add it to InputDecorationThemeData
There was a problem hiding this comment.
Let's only add it to InputDecorationThemeData (and InputDecoration). Newer properties like DialogThemeData.constraints and TabBarThemeData.splashBorderRadius were also only added to the data classes, not to DialogTheme / TabBarTheme. Nothing is lost, since ThemeData.inputDecorationTheme and InputDecorationTheme.of both use InputDecorationThemeData, so it can still be set locally with InputDecorationTheme(data: InputDecorationThemeData(...)). Could you remove it from InputDecorationTheme, and drop InputDecorationTheme from the changelog too? Sorry for the back and forth on the changelog!
…orting_text_padding.yaml Co-authored-by: Qun Cheng <[email protected]>
3 participants
This PR introduces the
supportingTextPaddingproperty toInputDecorator,InputDecoration, andInputDecorationThemeData.While originally conceived as
errorPadding, this property was renamed tosupportingTextPaddingduring review to align with the Material Design 3 specification, as it controls the padding for the entire supporting text row (which includes helper text, error text, and counter widgets).Additionally, this PR addresses layout, intrinsic sizing, and code consistency concerns raised during the review process:
Key Features & Refactors:
supportingTextPadding: Allows complete, independent control over the padding of the supporting text row (helper/error/counter).supportingTextConstraintsin_layout()to bound the supporting text row independently ofcontentConstraints, ensuring custom paddings do not break or shift the layout of inlineprefix/suffixwidgets.computeMinIntrinsicHeight()to computesupportingTextWidthseparately from the primary width deflation path, ensuring intrinsic height calculations accurately predict wrapped text heights.subtextreferences (e.g._SubtextSize,_computeSubtextSizes,subtextHeight) tosupportingTextto match the public property name and the M3 spec.List which issues are fixed by this PR. You must list at least one issue.
Port of #183582 which Fixes #175834
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot 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.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2