fix(ui): StreamButton icons picking up host app's ElevatedButtonThemeData.iconColor - #129
Conversation
…Data.iconColor Set `iconColor` alongside `foregroundColor` on the widget-level `ButtonStyle` so a host-app `ElevatedButtonThemeData.iconColor` cannot win over StreamButton's foreground via Flutter's `ButtonStyleButton.effectiveIconColor` resolution (which prefers `theme.iconColor` over `widget.foregroundColor`). Fixes GetStream/stream-chat-flutter#2786 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The file only pretty-printed SemanticsData for eyeball comparison against a Material `IconButton`; it never asserted anything, so it doesn't guard behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesStreamButton icon color fix
The color-resolution test and semantics comparison test files were removed. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
- Coverage 45.80% 44.53% -1.28%
==========================================
Files 178 178
Lines 7243 7243
==========================================
- Hits 3318 3226 -92
- Misses 3925 4017 +92 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stream_core_flutter/test/components/buttons/stream_button_test.dart (1)
164-192: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the regression test with a positive assertion on the expected icon color.
The current
expect(capturedIconColor, isNot(hostIconColor))is a negative-only check. IfcapturedIconColorwerenull(e.g., theBuilderclosure didn't execute orIconThemewasn't resolved),null != hostIconColoristrueand the test would pass silently. Asserting the actual expected foreground color would both guard against null and verify the icon uses the correct resolved color, not merely "not the host color."♻️ Suggested addition: also assert the expected foreground color
await tester.pumpAndSettle(); - expect(capturedIconColor, isNot(hostIconColor)); + expect(capturedIconColor, isNotNull); + expect(capturedIconColor, isNot(hostIconColor)); + + // Verify the icon uses StreamButton's resolved foreground color, + // not just "something other than the host color." + final button = tester.widget<ElevatedButton>(find.byType(ElevatedButton)); + final resolvedIconColor = button.style?.iconColor?.resolve({}); + expect(capturedIconColor, resolvedIconColor);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_core_flutter/test/components/buttons/stream_button_test.dart` around lines 164 - 192, Strengthen the assertion in the test “host-app ElevatedButtonThemeData.iconColor does not leak into StreamButton icons” by additionally verifying that capturedIconColor equals the expected resolved StreamButton foreground color. Keep the existing host-color inequality check if useful, and ensure the positive assertion rejects null while confirming the intended icon color.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/stream_core_flutter/test/components/buttons/stream_button_test.dart`:
- Around line 164-192: Strengthen the assertion in the test “host-app
ElevatedButtonThemeData.iconColor does not leak into StreamButton icons” by
additionally verifying that capturedIconColor equals the expected resolved
StreamButton foreground color. Keep the existing host-color inequality check if
useful, and ensure the positive assertion rejects null while confirming the
intended icon color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3beb0fe-14d1-45c1-ad0a-8f3a0fa324f7
📒 Files selected for processing (5)
packages/stream_core_flutter/CHANGELOG.mdpackages/stream_core_flutter/lib/src/components/buttons/stream_button.dartpackages/stream_core_flutter/test/components/buttons/stream_button_color_resolution_test.dartpackages/stream_core_flutter/test/components/buttons/stream_button_test.dartpackages/stream_core_flutter/test/components/buttons/stream_button_vs_icon_button_semantics_test.dart
💤 Files with no reviewable changes (2)
- packages/stream_core_flutter/test/components/buttons/stream_button_color_resolution_test.dart
- packages/stream_core_flutter/test/components/buttons/stream_button_vs_icon_button_semantics_test.dart
Summary
DefaultStreamButtonnow setsiconColoralongsideforegroundColoron its widget-levelButtonStyle, closing the leak where a host app'sElevatedButtonThemeData.iconColorwins over StreamButton's own foreground via Flutter'sButtonStyleButton.effectiveIconColorresolution (widget.iconColor ?? theme.iconColor ?? widget.foregroundColor ?? …).stream_button_test.dartthat pumps aStreamButton.iconwith a hostElevatedButtonThemeData(iconColor: …)and asserts the icon does not paint the leaked host color.stream_button_vs_icon_button_semantics_test.dart(print-only, no assertions).Fixes GetStream/stream-chat-flutter#2786
Test plan
flutter test test/components/buttons/stream_button_test.dart— all 7 tests pass.Summary by CodeRabbit
Bug Fixes
StreamButtonicons so they use the button’s configured foreground color instead of inheriting the host app’s button icon theme.Tests
StreamButtonicon colors.