Skip to content
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

[Material] Update TabController to support dynamic Tabs #30884

Merged
merged 15 commits into from Apr 29, 2019

Conversation

Projects
None yet
10 participants
@johnsonmh
Copy link
Contributor

commented Apr 11, 2019

Description

Because DefaultTabController creates its TabController once (in initState), it is currently impossible to:

  1. Modify the number of tabs you have after the first build.
  2. Load tabs dynamically/asynchronous.
  3. Update a TabBar and hot reload the changes.

This change adds an override to didUpdateWidget in DefaultTabController, it will detect updates to DefaultTabController.length and update the DefaultTabController.controller accordingly.

After the changes here, we can now do:
giphy

Related Issues

Tests

I added the following tests:

  • Build a TabBar with three tabs, update it to have two tabs, then one tab, then two tabs again. App should never crash and DefaultTabController should be updated accordingly.
  • When there is only one tab in a TabBar, it renders with the proper color (regression test for #15008).

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

johnsonmh added some commits Apr 10, 2019

@HansMuller

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

This looks like a good change: DefaultTabController.didUpdateWidget.didUpdateWidget definitely should DTRT when rebuilt with a new length.

I'd prefer to not make TabController.copyWith public. It introduces a problem with the lifetime the TabController's AnimationController, since TabController.dispose() will now apply to the original and all copies. Should be easy enough to deal with that in this one case, but I don't think we want to face the general problem (at least not yet).

@johnsonmh johnsonmh changed the title [WIP] Update TabController to support dynamic Tabs [Material] Update TabController to support dynamic Tabs Apr 24, 2019

johnsonmh added some commits Apr 24, 2019

@rami-a

rami-a approved these changes Apr 24, 2019

Copy link
Contributor

left a comment

LGTM, just small comments

johnsonmh added some commits Apr 25, 2019

@HansMuller
Copy link
Contributor

left a comment

Looks good with a few cleanup suggestions.

Show resolved Hide resolved packages/flutter/lib/src/material/tab_controller.dart Outdated
Show resolved Hide resolved packages/flutter/lib/src/material/tabs.dart Outdated
@HansMuller
Copy link
Contributor

left a comment

LGTM with one small change.

Show resolved Hide resolved packages/flutter/lib/src/material/tab_controller.dart Outdated

johnsonmh added some commits Apr 26, 2019

@johnsonmh johnsonmh merged commit 5412ef0 into flutter:master Apr 29, 2019

35 checks passed

WIP Ready for review
Details
add2app-macos Task Summary
Details
add2app-macos
Details
analyze Task Summary
Details
analyze
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-macos Task Summary
Details
build_tests-macos
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
deploy_gallery Task Summary
Details
deploy_gallery
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs Task Summary
Details
docs
Details
flutter-build
Details
integration_tests-linux Task Summary
Details
integration_tests-linux
Details
integration_tests-windows Task Summary
Details
integration_tests-windows
Details
tests-linux Task Summary
Details
tests-linux
Details
tests-macos Task Summary
Details
tests-macos
Details
tests-windows Task Summary
Details
tests-windows
Details
tool_tests-linux Task Summary
Details
tool_tests-linux
Details
tool_tests-macos Task Summary
Details
tool_tests-macos
Details
tool_tests-windows Task Summary
Details
tool_tests-windows
Details

@johnsonmh johnsonmh deleted the johnsonmh:fix-defaultTabController branch Apr 29, 2019

johnsonmh added a commit that referenced this pull request May 9, 2019

@shihaohong shihaohong referenced this pull request May 13, 2019

Merged

Tabs code/doc cleanup #32654

@JamalBelilet

This comment has been minimized.

Copy link

commented May 15, 2019

Can you please merge this to the dev branch.

@wzieba

This comment has been minimized.

Copy link

commented May 24, 2019

Thanks @johnsonmh for your work on this!

Can we know ETA for releasing this?

@fellow7000

This comment has been minimized.

Copy link

commented Jun 2, 2019

This will be great to have it in the very nearest future, stuck with this problem today :-/

@johnsonmh any news on that?..

@johnsonmh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Hi all! Glad this work is helpful to people.

This PR was merged sucessfully into master about a month ago. It is now following the regular release process. Since it's been about a month, it should be on the beta channel soon. And, keep in mind, you can start using this feature immediately by switching to the master channel.

@fellow7000

This comment has been minimized.

Copy link

commented Jun 3, 2019

@johnsonmh Hi! Thanks for prompt feedback!

Just changes to master and realized that this does not help me much, because I use TabController, not the DefaultTabController as I need to know the index of the selected Tab on screen.

Looks like your fix does not work for TabController (but it works for default one) :-/ Any ideas on that?

@johnsonmh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@fellow7000 Sorry, I don't know how you would make this work with a regular TabController.

If all you need is the index of the currently selected tab, you could do that pretty easily with a DefaultTabController by adding a int _selectedIndex on your state object, and update it in the TabBar.onTap callback.

Something like:

int _selectedIndex = 0;

...

DefaultTabController(
  length: length,
  child: TabBar(
    tabs: _tabs,
    onTap: (i) => setState(() => _selectedIndex = i),
  ),
),
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.