[Stepper] Add Alternative Label on horizontal-type Stepper#91496
[Stepper] Add Alternative Label on horizontal-type Stepper#91496craiglabenz merged 22 commits intoflutter:masterfrom
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. 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. |
|
I will add new tests to check this PR |
|
Thanks for the confirmation on tests, @KKimj. If you're new to widget tests, the existing Stepper widget tests contain many great examples of meaningful tests :) |
|
@craiglabenz |
|
Hello |
craiglabenz
left a comment
There was a problem hiding this comment.
Thanks so much for this PR, @KKimj :)
I left two comments, after which I think this will be good to go!
| /// Whether or not the step is active. The flag only influences styling. | ||
| final bool isActive; | ||
|
|
||
| /// [label] appears under the number icon. |
There was a problem hiding this comment.
I think this docstring is slightly misleading, as the [label] appears under the [title], not under the number icon. I recommend we write:
/// Optional widget that appears under the [title]. By default, uses the `bodyText1` theme.There was a problem hiding this comment.
Thanks!
I resolved it.
There was a problem hiding this comment.
Great! If we can now add another test to confirm the styling of the new elements, I think we'll be in good shape. (You can also add additional expect calls to your existing test.)
There was a problem hiding this comment.
Hello,
Is it right design?
// ...
late TextStyle captionStyle;
// on `StatefulBuilder(builder: (BuildContext context, StateSetter setState) {`
captionStyle = Theme.of(context).textTheme.caption!;
// on List<Step>
Step(
title: const Text('Step 3 Title'),
content: const Text('Step 3 Content'),
subtitle: const Text('Step 3 Subtitle'),
label: Text('Step 3 Label', style: Theme.of(context).textTheme.caption),
),
// ...
final Text label3Text = tester.widget<Text>(find.text('Step 3 Label'));
expect(captionStyle, label3Text.style);
Thanks!
There was a problem hiding this comment.
The lines that feel important to test (IMO) are those in _labelStyle, which make slight alterations to the TextTheme based on the state of the given step.
There was a problem hiding this comment.
@craiglabenz
Would you give me some comments?
Thanks!
There was a problem hiding this comment.
@craiglabenz
Would you change to status resolved?
Thanks!
| // Tap to Step2 Label then, index become 1 from 2 | ||
| await tester.tap(find.text('Step 2 Label')); | ||
| expect(index, 1); | ||
| }); |
There was a problem hiding this comment.
This is a good test! However, I think it would be nice to add another one that confirms the styling logic you've written is behaving as anticipated. There are quite a few good examples in this file for Button Themes, and should be portable to confirm the styling of Text widgets.
There was a problem hiding this comment.
@craiglabenz
Hello..! It's been so long. How have you been?
I pushed a commit for test about TextStyles of Label Widget.
Please Review my commit.
Thanks!
There was a problem hiding this comment.
@craiglabenz
Would you change to status resolved?
Thanks!
|
@craiglabenz |
|
I am So. So. sorry for disappearing for so long. Somehow, the alert for this PR snuck past me and I never caught it. In my last comment, I wrote "it would be nice to add another one that confirms the styling logic you've written is behaving as anticipated", which I now realize was a little unclear. The key function I would like to see tested is Does all of that make sense? PS - I promise not to disappear this time! |
|
Hello!! LTNS!! I will update test-codes soon.
P.S |
|
Yes, that looks like the right pseudocode. Thanks so much and 🌔 Happy New Year 🌔 to you, too! |
|
@KKimj Is this a PR you are interested in doing further work on? It's perfectly understandable if you don't have time right now, and if that's the case we can save what you've done so far and put it aside so that if someone else wants to work on this they can start from your PR. But if you do have the time to work on this, that would be great! |
|
@Hixie |
|
@craiglabenz I pushed a commit for test about Please Review my commit. |
|
That test looks good, @KKimj. However, I just tested the code myself directly, and the padding / alignment issue does still seem to be in play. Is this something you're also looking to address? |
|
@craiglabenz Now I realize again that there is no alternative label in the Vertical type, Anyway, I found a way to solve the Thanks, Have a good day! |
8cfc16d to
bbc79c6
Compare
c04e782 to
60586fc
Compare
|
@craiglabenz |
…1496) * Add label on Step * Add _buildLabelText, _labelStyle * Update _buildHorizontal with _buildLabelText * Fix Redundant Center widgets * Fix Unnecessary Column Widget * Fix Unnecessary import * Fix Unnecessary Container * Set label style to bodyText1 to match title style * Update test for Alternative Label * Fix code format * Update label docstring * Update to test textstyle of label * Update to Test `TextStyles` of Label * Fix typo in inline comments * Update for Stepper type to `horizontal` * Restore `_buildVerticalHeader` * Update docstring, `Only [StepperType.horizontal]` * Update for Readability * Add `_isLabel` method * Update `Horizontal height` with `_isLabel` * Update to make `_buildIcon` centered * Fix for `curly_braces_in_flow_control_structures`




Add Alternative Label on horizontal-type Stepper
Linked issues
#91337
Before
Screenshots
Horizontal
### Vertical
-
-
[important!] Remained issues
Padding, Align

Up and down number-icon

Reproduce code
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.