-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[Stepper] Add Alternative Label on horizontal-type Stepper #91496
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I resolved it.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@craiglabenz
Would you give me some comments?
Thanks!
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.