Skip to content

Update ListTile docs for color animation issues and add example #106955

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.

Sign up for GitHub

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

Jump to bottom
Merged
auto-submit merged 6 commits into from
Jul 13, 2022
Merged

Update ListTile docs for color animation issues and add example #106955

auto-submit merged 6 commits into from
Jul 13, 2022

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Jul 1, 2022 •
edited
Loading

context: #102310

part of #105760
related to #107104

This PR adds an explanation and an example of when ListTile needs to be wrapped with Material to animate colors.

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 d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Jul 1, 2022
@TahaTesser TahaTesser requested review from Piinks and gspencergoog July 1, 2022 14:11
@TahaTesser
Copy link
Member Author

TahaTesser commented Jul 1, 2022 •
edited
Loading

cc: @Piinks
This is part of the work left from #102310

@TahaTesser TahaTesser requested review from HansMuller and removed request for gspencergoog July 1, 2022 14:15
@TahaTesser TahaTesser force-pushed the update_list_tile_docs branch from b03a651 to 5857c45 Compare July 4, 2022 08:06
@TahaTesser TahaTesser mentioned this pull request Jul 5, 2022
Merged
8 tasks
gspencergoog
gspencergoog approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

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

Piinks
Piinks reviewed Jul 7, 2022
View reviewed changes
packages/flutter/lib/src/material/list_tile.dart Outdated
Copy link
Contributor

@Piinks Piinks Jul 7, 2022

Choose a reason for hiding this comment

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

may need

Do they need to be wrapped individually in order for these animations to work? what other options are there?

Copy link
Contributor

@Piinks Piinks Jul 7, 2022

Choose a reason for hiding this comment

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

I can't tell where this lines up with the performance drawbacks you have documented in your other PR, but it would be nice to make sure they are close by here. :)

Copy link
Member Author

@TahaTesser TahaTesser Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I place that performance line just below so if you're building the list_tile with Material for animation, you can reference performance implications right there below it.

https://github.com/flutter/flutter/pull/107104/files#diff-b399b26a1bee87ea31bd379fc20cabc50e9c70bcfe8e4dfd3c65c09ef2a5a80dR73

Copy link
Contributor

@Piinks Piinks Jul 8, 2022

Choose a reason for hiding this comment

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

Thank you for checking that!

Can you clarify may need? If it is necessary in every case, it should just say must, if otherwise, when would you not need it? Is there another way?

Copy link
Member Author

@TahaTesser TahaTesser Jul 11, 2022

Choose a reason for hiding this comment

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

In my testing, I found that it is always needed when animating colors as you want colors to be animated with the ListTile itself and get the desired results.

Update docs to to say must.

Copy link
Contributor

@Piinks Piinks Jul 12, 2022

Choose a reason for hiding this comment

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

Thanks! That is one that folks always catch me on. 😜

@TahaTesser TahaTesser requested review from Piinks and removed request for HansMuller July 11, 2022 14:31
Piinks
Piinks approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 12, 2022
@TahaTesser TahaTesser force-pushed the update_list_tile_docs branch from 22152de to 92d6e44 Compare July 13, 2022 06:42
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 13, 2022

Try again when the tree status has been applied to this PR.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2022
@auto-submit auto-submit bot merged commit c251856 into flutter:master Jul 13, 2022
@TahaTesser TahaTesser deleted the update_list_tile_docs branch July 13, 2022 08:21
This was referenced Jul 13, 2022
Closed
Closed
This was referenced Jul 13, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 13, 2022
@engine-flutter-autoroll
c251856 Update ListTile docs for color animation issues and add exa…
657c5ab 
…mple (flutter/flutter#106955)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jul 13, 2022
@engine-flutter-autoroll
c251856 Update ListTile docs for color animation issues and add exa…
760a616 
…mple (flutter/flutter#106955)
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@TahaTesser @camsim99
Update ListTile docs for color animation issues and add example (fl…
7aba6ee 
…utter#106955)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gspencergoog gspencergoog gspencergoog approved these changes

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TahaTesser @gspencergoog @Piinks