Enhance the Stepper widget by adding customizable header and content padding#180257
Conversation
…ing properties. Default values are set for both paddings to maintain existing layout behavior.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request introduces headerPadding and contentPadding to the Stepper widget, providing more customization options. The changes are well-implemented and use appropriate defaults. I've provided one suggestion to improve the accuracy of the documentation for the new contentPadding property to ensure developers have a clear understanding of its behavior.
| /// For [StepperType.vertical], defaults to | ||
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. |
There was a problem hiding this comment.
The documentation for the default contentPadding in a vertical stepper is slightly inaccurate. The start padding is 60.0 plus the left value of stepIconMargin, if provided. Please update the documentation to reflect this for better clarity.
| /// For [StepperType.vertical], defaults to | |
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. | |
| /// For [StepperType.vertical], defaults to | |
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. The `start` padding is also increased by the `left` value of [stepIconMargin] if it is provided. |
References
- The style guide states that documentation should be useful. The current documentation for
contentPaddingis slightly misleading about its default value, which reduces its usefulness. It should be updated for accuracy. (link)
There was a problem hiding this comment.
What do you think of this? Or maybe always add marginLeft onto contentPadding.start? I'll leave it to you to decide which feels the most reasonable.
There was a problem hiding this comment.
Yes, I think always adding marginLeft to contentPadding.start is the more reasonable approach.
dkwingsmt
left a comment
There was a problem hiding this comment.
Thanks for the contribution. It generally looks good to me. Can you add some unit tests?
| /// The padding around the header row in both [StepperType.vertical] and | ||
| /// [StepperType.horizontal] steppers. | ||
| /// | ||
| /// If null, defaults to `EdgeInsets.symmetric(horizontal: 24.0)`. |
There was a problem hiding this comment.
| /// If null, defaults to `EdgeInsets.symmetric(horizontal: 24.0)`. | |
| /// Defaults to `EdgeInsets.symmetric(horizontal: 24.0)`. |
| /// For [StepperType.vertical], defaults to | ||
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. |
There was a problem hiding this comment.
What do you think of this? Or maybe always add marginLeft onto contentPadding.start? I'll leave it to you to decide which feels the most reasonable.
Sure I can |
…. Added tests for customizable header and content padding for both vertical and horizontal steppers, ensuring proper alignment with step icons when margins are applied.
|
Hello @dkwingsmt, I've committed the recommended changes and added unit tests. |
| ); | ||
|
|
||
| final Stepper stepper = tester.widget<Stepper>(find.byType(Stepper)); | ||
| expect(stepper.headerPadding, customPadding); |
There was a problem hiding this comment.
This test is doing nothing. It constructs an object, then test that the object has the right property, which is trivial and involves nothing about the changed code.
You should instead test that the components are placed at the expected coordinate.
There was a problem hiding this comment.
Updated the tests to validate layout behavior rather than property values. The tests are now validating the position of the displayed header as well as the application of the padding.
dkwingsmt
left a comment
There was a problem hiding this comment.
Generally LGTM except for some minor issues. Thank you!
| /// For [StepperType.vertical], defaults to | ||
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. The `start` padding is also increased by the `left` value of [stepIconMargin] if it is provided. |
There was a problem hiding this comment.
| /// For [StepperType.vertical], defaults to | |
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. The `start` padding is also increased by the `left` value of [stepIconMargin] if it is provided. | |
| /// For [StepperType.vertical], defaults to | |
| /// `EdgeInsetsDirectional.only(start: 60.0, end: 24.0, bottom: 24.0)`. | |
| /// The `start` padding is also increased by the `left` value of | |
| /// [stepIconMargin] if it is provided. |
Warp lines.
| final double? additionalMarginLeft = marginLeft != null ? marginLeft / 2.0 : null; | ||
| final double? additionalMarginRight = marginRight != null ? marginRight / 2.0 : null; | ||
| // Default content padding for vertical stepper. | ||
| const EdgeInsetsDirectional defaultContentPadding = EdgeInsetsDirectional.only( |
There was a problem hiding this comment.
Make this a global called `_kDefaultContentPadding
|
|
||
| final Rect customTitleRect = tester.getRect(find.text('Step 1')); | ||
|
|
||
| expect(customTitleRect.left, lessThan(defaultTitleRect.left)); |
There was a problem hiding this comment.
Can you test the .top as well? (I suppose that's also affected by padding?)
|
|
||
| final Rect customTitleRect = tester.getRect(find.text('Step 1')); | ||
|
|
||
| expect(customTitleRect.left, lessThan(defaultTitleRect.left)); |
| moreOrLessEquals(8.0)); | ||
| }); | ||
|
|
||
| testWidgets('Stepper default headerPadding', (WidgetTester tester) async { |
There was a problem hiding this comment.
Shall we remove this test case, since the previous test cases are sufficient? Testing widget property isn't useful.
There was a problem hiding this comment.
Sure, will remove
|
Also, the analyzer check is failing. |
| elevation: widget.elevation ?? 2, | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 24.0), | ||
| padding: widget.headerPadding ?? const EdgeInsets.symmetric(horizontal: 24.0), |
There was a problem hiding this comment.
Is this a case for an effectiveHeaderPadding that can be reused elsewhere within the class, and a _kDefaultHeaderPadding?
There was a problem hiding this comment.
Done! Added _kDefaultHeaderPadding constant and effectiveHeaderPadding getter, matching the contentPadding pattern. Updated both usages to use the getter.
…nt, improving code clarity and maintainability. This change ensures consistent padding behavior across vertical steppers.
Verify that headerPadding affects both horizontal (.left) and vertical (.top) positioning for vertical and horizontal stepper types.
Extract default value into constant and create effectiveHeaderPadding getter, matching the contentPadding pattern. Removes duplication in _buildVerticalHeader and _buildHorizontal methods.
… clarifying the default padding behavior for vertical steppers. This change enhances code readability and maintains consistency in padding logic.
Remove explicit type annotations from const declarations where types can be inferred.
…ected EdgeInsets, allowing type inference. This change aligns with recent linter recommendations for cleaner code.
Done @dkwingsmt |
| controller: widget.controller, | ||
| physics: widget.physics, | ||
| padding: const EdgeInsets.all(24.0), | ||
| padding: widget.contentPadding ?? const EdgeInsets.all(24.0), |
There was a problem hiding this comment.
Can we make this fallback a global? I'm wondering if the already defined constant _kDefaultContentPadding is maybe not appropriate given the contentPadding has different fallbacks in different situations.
…padding (flutter#180257) Enhance the Stepper widget by adding customizable header and content padding properties. Default values are set for both paddings to maintain the existing layout behavior. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *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.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## 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. 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 [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 <dkwingsmt@users.noreply.github.com>
…padding (flutter#180257) Enhance the Stepper widget by adding customizable header and content padding properties. Default values are set for both paddings to maintain the existing layout behavior. <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> *Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.* *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.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## 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. 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 [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 <dkwingsmt@users.noreply.github.com>
Enhance the Stepper widget by adding customizable header and content padding properties. Default values are set for both paddings to maintain the existing layout behavior.
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
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.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).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.