Skip to content

Allow detection of taps on TabBar - #23919

Merged
HansMuller merged 16 commits into
flutter:masterfrom
JaspervanRiet:tabbar-allow-taps
Dec 13, 2018
Merged

HansMuller merged 16 commits into
flutter:masterfrom
JaspervanRiet:tabbar-allow-taps

Conversation

@JaspervanRiet

@JaspervanRiet JaspervanRiet commented Nov 4, 2018 •

Copy link
Copy Markdown
Contributor

Fixes #24224

I have added a callback to the TabBar that is called if not null. This would fix all 3 of these examples.

@Hixie

Hixie commented Nov 11, 2018

Copy link
Copy Markdown
Contributor

This will need a test.

@Hixie

Hixie commented Nov 11, 2018

Copy link
Copy Markdown
Contributor

Generally speaking I'd like to keep callbacks such that not setting one and setting one to a no-op callback (as in () { }) do the same thing (i.e. adding a no-op callback shouldn't change behaviour). It can be a bit unintuitive if adding something that does nothing does something. :-)

Not really sure what the best way to do that here would be, though...

cc @HansMuller

@JaspervanRiet

Copy link
Copy Markdown
Contributor Author

I have opened an issue so there can be proper discussion (should have done this before) #24224. A potential solution would be to handle this in the _handleTap function, first let the TabBar handle the tap, and then invoke a callback to the user if non null.

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Nov 28, 2018
/// is null then the text style of the theme's body2 definition is used.
final TextStyle unselectedLabelStyle;

/// An option callback when the [TabBar] is tapped.

@HansMuller HansMuller Nov 30, 2018 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An optional callback that's called when the [TabBar] is tapped.

The callback is applied to the index of the tab where the tap occurred.

This callback has no effect on the default handling of taps. It's for applications that want to do a little extra work when a tab is tapped, even if the tap doesn't change the TabController's index. TabBar [onTap] callbacks should not make changes to the TabController since that would interfere with the default tap handler.

child: TabBar(
controller: controller,
tabs: tabs.map<Widget>((String tab) => Tab(text: tab)).toList(),
onTap: (int index) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

need to fix the indent here

controller: controller,
tabs: tabs.map<Widget>((String tab) => Tab(text: tab)).toList(),
onTap: (int index) {
if (controller.index != index) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is what we don't want developers to do; onTap is a "me too" notification.

Maybe this is leftover from a previous version of the PR?

semantics.dispose();
});

testWidgets('can be notified of TabBar onTap behavior', (WidgetTester tester) async {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test looks like it needs to be revised. It should confirm that the onTap() call is applied to the correct index and that it happens even when the default tap handler doesn't have any effect on the TabController.

@JaspervanRiet

Copy link
Copy Markdown
Contributor Author

@HansMuller Thanks for the review, I believe these have now been resolved.

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good, just needs a few trivial changes.

///
/// This callback has no effect on the default handling of taps. It's for
/// applications that want to do a little extra work when a tab is tapped,
/// even if the tap doesn't change the TabController's index. TabBar [onTap]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extra leading space on this line and the next

await tester.pumpWidget(buildFrame(tabs: tabs, controller: controller));
await tester.tap(find.text('C'));
await tester.pump();
await tester.pump(const Duration(seconds: 1));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tester.pumpAndSetttle() here and below


testWidgets('can be notified of TabBar onTap behavior', (WidgetTester tester) async {
int tabIndex = -1;
bool hasBeenTapped = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This appears to be redundant with tabIndex != -1 and existing expects already check that.

@HansMuller HansMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

I will land this as soon as the repo is open again.

@HansMuller
HansMuller merged commit 01694ab into flutter:master Dec 13, 2018
justinmc added a commit that referenced this pull request Dec 13, 2018
justinmc added a commit that referenced this pull request Dec 17, 2018
…#25342)

* Revert "Add imports section to sample code templates, and more docs. (#25184)"

This reverts commit 95b0124.

* Revert "Use stderr instead of stdout to contain errors in flutter attach test (#25305)"

This reverts commit 2b819dd.

* Revert "Allow detection of taps on TabBar (#23919)"

This reverts commit 01694ab.

* Revert "a549981da Roll src/third_party/skia a69b10312977..5eb29448dfbd (1 commits) (flutter-team-archive/engine#7211) (#25333)"

This reverts commit a37099f.

* Revert "Revert "obscureText and enableInteractiveSelection defaults (#24527)" (#25335)"

This reverts commit c545706.

* Password fields are no longer selectable nor copiable
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TabBar does not allow detection of taps

5 participants