Skip to content

Conversation

@KKimj
Copy link
Contributor

@KKimj KKimj commented Oct 8, 2021

Add Alternative Label on horizontal-type Stepper

Linked issues

#91337

Before

image


Screenshots

Horizontal

image

### Vertical

image

[important!] Remained issues

  • Padding, Align
    image

  • Up and down number-icon
    image

Reproduce code

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      title: 'Stepper Alternative Label',
      home: MyStatefulWidget(),
    );
  }
}

class MyStatefulWidget extends StatefulWidget {
  const MyStatefulWidget({Key? key}) : super(key: key);

  @override
  State<MyStatefulWidget> createState() => _MyStatefulWidgetState();
}

class _MyStatefulWidgetState extends State<MyStatefulWidget> {
  int _index = 0;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Stepper(
        type: StepperType.horizontal,
        // type: StepperType.vertical,
        currentStep: _index,
        onStepCancel: () {
          if (_index > 0) {
            setState(() {
              _index -= 1;
            });
          }
        },
        onStepContinue: () {
          if (_index < 3) {
            setState(() {
              _index += 1;
            });
          }
        },
        onStepTapped: (int index) {
          setState(() {
            _index = index;
          });
        },
        steps: <Step>[
          Step(
            isActive: _index == 0,
            title: const Text('Step 1 title'),
            content: Container(
                alignment: Alignment.centerLeft,
                child: const Text('Content for Step 1')),
            label: const Text('Step 1'),
            subtitle: const Text('Step 1 subtitle'),
          ),
          Step(
            isActive: _index == 1,
            title: const Text('Step 2 title'),
            content: const Text('Content for Step 2'),
            subtitle: const Text('Step 2 subtitle'),
            label: const Text('Step 2'),
          ),
          Step(
            isActive: _index == 2,
            title: const Text('Step 3 title'),
            content: const Text('Content for Step 3'),
            subtitle: const Text('Step 3 subtitle'),
            label: const Text('Step 3'),
          ),
          Step(
            isActive: _index == 3,
            title: const Text('Step 4 title'),
            content: const Text('Content for Step 4'),
            subtitle: const Text('Step 4 subtitle'),
            label: const Text('Step 4'),
          ),
        ],
      ),
    );
  }
}

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 8, 2021
@google-cla google-cla bot added the cla: yes label Oct 8, 2021
@KKimj KKimj changed the title Stepper alternative label [Stepper] Add Alternative Label Oct 8, 2021
@KKimj KKimj marked this pull request as ready for review October 8, 2021 16:23
@flutter-dashboard
Copy link

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.

@KKimj
Copy link
Contributor Author

KKimj commented Oct 8, 2021

I will add new tests to check this PR

@HansMuller HansMuller requested a review from craiglabenz October 8, 2021 21:48
@craiglabenz
Copy link
Contributor

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 :)

@KKimj
Copy link
Contributor Author

KKimj commented Oct 9, 2021

@craiglabenz
Hello, I add new test for lable ( Alternative Label ) with stepper.
Please check, and give me some comments.
Thanks, HAGW!

@KKimj
Copy link
Contributor Author

KKimj commented Oct 17, 2021

@craiglabenz

Hello
Can you please comment on my PR?
Thank you.

Copy link
Contributor

@craiglabenz craiglabenz left a 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.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I resolved it.

Copy link
Contributor

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.)

Copy link
Contributor Author

@KKimj KKimj Oct 19, 2021

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!

Copy link
Contributor

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.

Copy link
Contributor Author

@KKimj KKimj Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

There are _titleStyle, _subtitleStyle, _labelStyle methods, in order to indicate state of Step with TextStyles.

Thanks!

Copy link
Contributor Author

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!

Copy link
Contributor Author

@KKimj KKimj Jun 4, 2022

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);
});
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor Author

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!

@KKimj
Copy link
Contributor Author

KKimj commented Nov 13, 2021

@craiglabenz
Hello, how are you doing?
Can you give me a new feedback?
Thank you.

@craiglabenz
Copy link
Contributor

craiglabenz commented Jan 26, 2022

@KKimj

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 _labelStyle, since it accepts an index and merges that with the user's progress through the stepper and the Dark Mode / Light Mode status before ultimately yielding a TextStyle. The internals of that function are pretty complicated, so I think we should have tests to verify that a Stepper with X steps, and currently at step Y, is rendering each label correctly. There are a few scenarios to cover there - steps before, on, and after the active step; in both light mode and dark mode.

Does all of that make sense?

PS - I promise not to disappear this time!

@KKimj
Copy link
Contributor Author

KKimj commented Jan 31, 2022

Hello!! LTNS!!
I understand what you say.

I will update test-codes soon.
Something like below.

  • Let, X := currentStepIndex, Y := arbitraryStepIndex (except current)
  1. set X = 0.
  2. Check Styles of X and Y.
  3. set X = 1.
  4. Invoke setState().
  5. Check Styles of X and Y.

P.S
Happy New Year!

@craiglabenz
Copy link
Contributor

Yes, that looks like the right pseudocode. Thanks so much and 🌔 Happy New Year 🌔 to you, too!

@Hixie
Copy link
Contributor

Hixie commented Apr 19, 2022

@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!

@KKimj
Copy link
Contributor Author

KKimj commented Apr 19, 2022

@Hixie
Hello!
I always had it in mind. I want to finish all the work within April. How does that sound?

@KKimj
Copy link
Contributor Author

KKimj commented Apr 21, 2022

@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!

@craiglabenz
Copy link
Contributor

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?

@KKimj
Copy link
Contributor Author

KKimj commented Apr 25, 2022

@craiglabenz
Hello!

Now I realize again that there is no alternative label in the Vertical type, StepperType.vertical.

Anyway, I found a way to solve the padding / alignment issue.
The method is to modify the function '_buildVerticalHeader', forcing width to 24.0, according to the Material Guide.

  • Code diff here, (It was not pushed.)
  • image
  • An example UI can be similar to the following.

image

But it's too cramped, and as I said above, it's dubious because `alternative label` is not in the Vertical type. I'd like to ask for advice on how to do it.

Thanks, Have a good day!

@google-cla google-cla bot added cla: no and removed cla: yes labels Apr 25, 2022
@KKimj KKimj force-pushed the stepper-alternative-label branch from 8cfc16d to bbc79c6 Compare April 25, 2022 15:49
@google-cla google-cla bot added cla: yes and removed cla: no labels Apr 25, 2022
@KKimj KKimj force-pushed the stepper-alternative-label branch from c04e782 to 60586fc Compare June 6, 2022 15:53
@KKimj
Copy link
Contributor Author

KKimj commented Jun 6, 2022

@craiglabenz
Thanks!
It works!

@craiglabenz craiglabenz merged commit a939d9d into flutter:master Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
…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`
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
@KKimj KKimj mentioned this pull request Apr 18, 2023
8 tasks
auto-submit bot pushed a commit that referenced this pull request Apr 18, 2023
Adding my name (Kim Jiun) to Authors.

REF : #91496
Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants