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.

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

Feat: Animate fill for material app bar #163913

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feat: Animate fill for material app bar #163913

wants to merge 1 commit into from
+49 −1

Conversation

rkishan516
Copy link
Contributor

@rkishan516 rkishan516 commented Feb 22, 2025 •
edited
Loading

Feat: Animate fill for material app bar
fixes: #162988

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Feb 22, 2025
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #163913 at sha a3352c9

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Feb 22, 2025
@dkwingsmt
Copy link
Contributor

Thanks for the change. Although I do see how it's improving the fidelity, it probably shouldn't be implemented on Material, which controls all widgets, but instead should be in the app bar widget.

Also we'll need some tests to guarantee it's not broken after the merge.

@rkishan516
Copy link
Contributor Author

@dkwingsmt Its ready to review again.

dkwingsmt
dkwingsmt reviewed Mar 18, 2025
View reviewed changes
packages/flutter/test/material/app_bar_test.dart

expect(getAppBarAnimatedBackgroundColor(tester), defaultColor);
await tester.pumpAndSettle();
expect(getAppBarAnimatedBackgroundColor(tester), scrolledColor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test case verifies the feature. It only verifies the starting color and ending color, while you're trying to verify that there's a smooth animation in between. I recommend going without pumpAndSettle but pumping a time instead, and it should result in a intermediate color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Let me do the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed changes for 2 intermediate color.

  1. At 50ms after animation start
  2. At 100ms after animation start

Animation duration is 200ms.

@rkishan516 rkishan516 force-pushed the animate-app-bar-fill branch 4 times, most recently from b1a60d1 to 5144d7a Compare March 18, 2025 10:07
@dkwingsmt
Copy link
Contributor

The errors seem to be caused by comparing equality of the floating points. Try isSameColorAs, which allows tolerance. (Also therefore you don't need that much precision)

@rkishan516 rkishan516 force-pushed the animate-app-bar-fill branch from 5144d7a to a646e41 Compare March 18, 2025 22:06
@rkishan516
Copy link
Contributor Author

The errors seem to be caused by comparing equality of the floating points. Try isSameColorAs, which allows tolerance. (Also therefore you don't need that much precision)

Thanks, this worked very well.

@dkwingsmt dkwingsmt requested a review from QuncCccccc March 19, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkwingsmt dkwingsmt dkwingsmt left review comments

@QuncCccccc QuncCccccc Awaiting requested review from QuncCccccc

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Material AppBar fill on scroll should be animated
2 participants
@rkishan516 @dkwingsmt