Skip to content

Allow ListTiles to be autofocused#54229

Merged
gspencergoog merged 1 commit into
flutter:masterfrom
EricEnslen:autofocus_list_tiles
Apr 7, 2020
Merged

Allow ListTiles to be autofocused#54229
gspencergoog merged 1 commit into
flutter:masterfrom
EricEnslen:autofocus_list_tiles

Conversation

@EricEnslen

Copy link
Copy Markdown
Contributor

Description

This PR adds an "autofocus" param to ListTile and its Checkbox, Radio and Switch variants, and passes the given value through to the wrapped InkWell, Switch, etc.

This is important for scenarios like a settings screen, where the first interactable element on a page may be a SwitchListTile, for example.

Related Issues

N/A (trivial effort)

Tests

I added the following tests:

Added a test to each class that verifies setting the new autofocus param to true will focus the Tile, unless the tile is disabled.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

This is important for scenarios like a settings screen,
where the first interactable element on a page may be
a SwitchListTile, for example.
this.onTap,
this.onLongPress,
this.selected = false,
this.autofocus = 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.

How about also adding focusColor, hoverColor, and focusNode here, for parity with other widgets? You'd just have to pass them to the InkWell, so there's nothing to implement, really.

You're welcome to crib the tests for the colors from here: https://github.com/flutter/flutter/pull/53888/files#diff-557cb6cb0346a07994b565cb2a524544R1191

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.

Also, of course, if you don't have time for that, I'm happy to just add them in mine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, could you please add them in yours? It might take me a while to add those to all 4 classes.

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.

Sure, no problem.

@gspencergoog gspencergoog 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.

Thanks for the PR!

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@gspencergoog
gspencergoog merged commit cccfe96 into flutter:master Apr 7, 2020
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants