Replace Material icons with custom SVG icon system #80
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add WnIcon widget with WnIcons enum for type-safe icon usage - Replace all Icons.xxx with WnIcon(WnIcons.xxx, ...) across codebase - Add 70+ new SVG icons to assets/svgs/ - Update WnWarningBox to accept WnIcons instead of IconData - Refactor _SettingsTile to use WnIcons for consistency - Update test files to use WnIcon-based finders Closes #14 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
erskingardner
force-pushed
the
svg-icons
branch
from
acd4f49 to
3b3cbc5
Compare
WalkthroughAdds a new WnIcon widget and WnIcons enum and migrates the codebase from Material Icons / inline SVG usage to the new SVG-based WnIcon system across many screens, widgets, and tests. Adjusts some private widget constructors to accept WnIcons instead of svg path or IconData. Changes
Sequence Diagram(s)(omitted — changes are icon/widget replacements without multi-component control-flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
➡️ Coverage: 99.62% → 99.62% (0.00%) |
1 similar comment
➡️ Coverage: 99.62% → 99.62% (0.00%) |
- Test WnIcons enum path generation - Test unique filenames across all icons - Test widget rendering, sizing, and color filtering Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/widgets/wn_account_bar.dart (1)
36-49: UseWnIconhere to keep the icon system centralized.This button still renders the SVG directly; swapping to
WnIcon(WnIcons.newChat)keeps the icon system consistent and removes duplicated color-filter logic.♻️ Proposed refactor
-import 'package:flutter_svg/flutter_svg.dart'; +import 'package:sloth/widgets/wn_icon.dart'; ... - icon: SvgPicture.asset( - 'assets/svgs/new_chat.svg', - width: 24.w, - height: 24.w, - colorFilter: ColorFilter.mode( - colors.backgroundContentPrimary, - BlendMode.srcIn, - ), - ), + icon: WnIcon( + WnIcons.newChat, + size: 24.w, + color: colors.backgroundContentPrimary, + ),lib/widgets/wn_chat_header.dart (2)
30-36: Remove tooltip from IconButton.The back button icon migration to
WnIcon(WnIcons.chevronLeft)is correct. However, based on learnings, tooltips should not be used for UI elements in this project. Thetooltip: 'Back'on line 35 should be removed.Proposed fix
IconButton( key: const Key('back_button'), onPressed: onBack, icon: WnIcon( WnIcons.chevronLeft, color: colors.backgroundContentTertiary, size: 28.w, ), - tooltip: 'Back', ),
55-65: TODO acknowledged; also remove tooltip from menu button.The TODO comment for the pending SVG icon is appropriate. However, the
tooltip: 'Menu'on line 64 should also be removed per the project guideline against tooltips.Proposed fix
// TODO: Replace with SVG icon when more_horizontal.svg is available IconButton( key: const Key('menu_button'), onPressed: onMenuTap, icon: Icon( Icons.more_horiz, color: colors.backgroundContentTertiary, size: 24.w, ), - tooltip: 'Menu', ),
🤖 Fix all issues with AI agents
In `@lib/screens/chat_screen.dart`:
- Around line 238-242: The icon inside the fixed-size send button is not
centered because WnIcon (WnIcons.arrowUp) is placed directly in the 44.w×44.h
Container; wrap the WnIcon in a Center widget (the same pattern used in
wn_text_form_field.dart paste button) so the SvgPicture returned by WnIcon is
horizontally and vertically centered within the button container.
In `@lib/screens/settings_screen.dart`:
- Around line 144-149: The _SettingsTile constructor lacks a Key parameter;
update the const _SettingsTile({...}) constructor to accept an optional Key
(e.g. Key? key) and forward it to the superclass via super.key to enable widget
testing with find.byKey; update any instantiations of _SettingsTile if necessary
to pass keys for tests (refer to class name _SettingsTile and its constructor
parameters icon, label, onTap).
In `@lib/widgets/wn_icon.dart`:
- Around line 101-109: The build method in wn_icon.dart currently only applies a
ColorFilter when the local color is set, losing IconTheme behavior; update the
color selection to fall back to IconTheme.of(context).color (i.e., use final
resolvedColor = color ?? IconTheme.of(context).color) and then pass colorFilter:
resolvedColor != null ? ColorFilter.mode(resolvedColor, BlendMode.srcIn) : null
when creating the SvgPicture.asset (refer to build, size, icon.path, and color
variables).
In `@test/screens/edit_profile_screen_test.dart`:
- Around line 133-134: Replace the fragile finder that uses
find.byType(WnIcon).first with a key-based finder: add a Key (e.g.,
Key('close_button')) to the close button widget in the UI code, then in this
test replace the closeIcon assignment to use find.byKey(const
Key('close_button')) and tap that; reference the WnIcon close button instance to
add the key and update this test (and mirror the same change you applied in
profile_keys_screen_test.dart).
In `@test/screens/profile_keys_screen_test.dart`:
- Around line 89-90: The test is using a fragile selector
find.byType(WnIcon).first for the close button; add a stable key to the close
button widget (assign Key('close_button') to the WnIcon instance used as the
close button in the screen code) and update the test to use find.byKey(const
Key('close_button')) (replace the closeIcon finder and the tester.tap call
accordingly) so the test targets the explicit close_button key instead of the
first WnIcon.
In `@test/screens/sign_out_screen_test.dart`:
- Around line 146-147: The test is using a fragile selector
(find.byType(WnIcon).first) to tap the close icon; add a stable Key to the close
button widget (e.g., Key('sign_out_close_button')) in the UI code where the
WnIcon close control is created, then update the test to locate it via
find.byKey(const Key('sign_out_close_button')) instead of
find.byType(WnIcon).first (replace the closeIcon variable and the tester.tap
call accordingly).
Comment on lines
144
to
149
| class _SettingsTile extends StatelessWidget { | ||
| const _SettingsTile({ | ||
| required this.svgPath, | ||
| required this.icon, | ||
| required this.label, | ||
| required this.onTap, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding super.key for testability.
The constructor lacks key support. While this private widget can be tested via label text, adding key support would improve consistency with the project's testing approach.
🔧 Suggested improvement
class _SettingsTile extends StatelessWidget {
const _SettingsTile({
+ super.key,
required this.icon,
required this.label,
required this.onTap,
});Based on learnings, the project prefers using find.byKey() in tests.
🤖 Prompt for AI Agents
In `@lib/screens/settings_screen.dart` around lines 144 - 149, The _SettingsTile
constructor lacks a Key parameter; update the const _SettingsTile({...})
constructor to accept an optional Key (e.g. Key? key) and forward it to the
superclass via super.key to enable widget testing with find.byKey; update any
instantiations of _SettingsTile if necessary to pass keys for tests (refer to
class name _SettingsTile and its constructor parameters icon, label, onTap).
➡️ Coverage: 99.62% → 99.62% (0.00%) |
… test finders - Wrap send button WnIcon in Center widget for proper positioning - Add IconTheme.of(context).color fallback for theme-aware icon coloring - Update test files to use key-based finders instead of fragile type finders - Add tests for IconTheme behavior in WnIcon widget Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
➡️ Coverage: 99.62% → 99.62% (0.00%) |
Apply disabled-aware color to copy icon so it dims when npub is null, matching standard IconButton behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/screens/profile_keys_screen_test.dart`:
- Around line 187-210: The test currently inspects all WnIcon widgets which can
yield false positives; narrow the assertions to the icon inside the visibility
toggle: use the existing visibilityToggle Finder (Key 'visibility_toggle') and
locate its descendant WnIcon (e.g., via find.descendant or
find.byWidgetPredicate scoped to visibilityToggle), then read the icon value on
that specific WnIcon and assert equality with WnIcons.view and WnIcons.viewOff
during the two toggle states (use the widget obtained from tester.widget or
.evaluate().single.widget cast to WnIcon).
In `@test/widgets/wn_icon_test.dart`:
- Around line 99-105: The test name is misleading: it says “has no color filter”
but only checks rendering; update the test to either assert the absence of a
color filter on the SvgPicture or rename the test to reflect its actual check.
For a minimal change, rename the test registered by testWidgets to something
like "renders when neither color nor IconTheme is set" and keep the existing
verification that WnIcon(WnIcons.warning) mounts and findsOneWidget of
SvgPicture; alternatively, if you prefer to assert behavior, add an assertion
that the found SvgPicture's color filter (or equivalent property) is null/absent
after mounting WnIcon to confirm no color filtering is applied.
❌ Coverage: 99.62% → 99.59% (-0.03%) |
- WnIcon: size edge cases, color variations, nested IconTheme inheritance - Theme: SemanticColors copyWith/lerp, AccentColorSet operations - SemanticAccentColors: all 12 colors, lerp transitions - BuildContext extension: light/dark themes, fallback behavior - Profile keys test: scope visibility icon finder with find.descendant - wn_icon test: rename misleading test name Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/widgets/wn_icon_test.dart`:
- Around line 201-202: The variables parentIcon and childIcon in the test are
fetched via tester.widget<WnIcon>(find.byKey(...)) but never used except to
assert their .icon properties later; remove these unused local variables and
instead perform the icon assertions inline (e.g.,
assertEquals(tester.widget<WnIcon>(find.byKey(const Key('parent_icon'))).icon,
...)) or delete the fetch lines if the key-based find already suffices; update
references to parentIcon/childIcon to use inline calls to
tester.widget<WnIcon>(find.byKey(...)) or remove them entirely.
Comment on lines
+201
to
+202
| final parentIcon = tester.widget<WnIcon>(find.byKey(const Key('parent_icon'))); | ||
| final childIcon = tester.widget<WnIcon>(find.byKey(const Key('child_icon'))); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused variables parentIcon and childIcon.
These variables are fetched but only their .icon property is asserted (lines 218-219). The assertions could be done inline without storing the intermediate variables, or these lines could be removed if .icon checks are sufficient given the key-based lookup already confirms identity.
♻️ Optional cleanup
- final parentIcon = tester.widget<WnIcon>(find.byKey(const Key('parent_icon')));
- final childIcon = tester.widget<WnIcon>(find.byKey(const Key('child_icon')));
-
// Find SVG pictures by their parent WnIcon keys
final parentSvg = tester.widget<SvgPicture>(
find.descendant(
@@ -215,8 +212,8 @@
),
);
- expect(parentIcon.icon, WnIcons.warning);
- expect(childIcon.icon, WnIcons.search);
+ expect(tester.widget<WnIcon>(find.byKey(const Key('parent_icon'))).icon, WnIcons.warning);
+ expect(tester.widget<WnIcon>(find.byKey(const Key('child_icon'))).icon, WnIcons.search);
expect(
parentSvg.colorFilter,
const ColorFilter.mode(parentColor, BlendMode.srcIn),🤖 Prompt for AI Agents
In `@test/widgets/wn_icon_test.dart` around lines 201 - 202, The variables
parentIcon and childIcon in the test are fetched via
tester.widget<WnIcon>(find.byKey(...)) but never used except to assert their
.icon properties later; remove these unused local variables and instead perform
the icon assertions inline (e.g.,
assertEquals(tester.widget<WnIcon>(find.byKey(const Key('parent_icon'))).icon,
...)) or delete the fetch lines if the key-based find already suffices; update
references to parentIcon/childIcon to use inline calls to
tester.widget<WnIcon>(find.byKey(...)) or remove them entirely.
❌ Coverage: 99.62% → 99.59% (-0.03%) |
❌ Coverage: 99.62% → 99.59% (-0.03%) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/widgets/wn_icon_test.dart`:
- Around line 253-286: The orphaned SVG detection fails on Windows because
svgFiles uses f.path.split('/').last; update the test to import package:path
(e.g., import 'package:path/path.dart' as p) and replace the split('/') call
with p.basename(f.path).replaceAll('.svg', '') so paths are normalized
cross-platform, and add path: ^1.9.0 to dev_dependencies in pubspec.yaml; modify
the test group around WnIcons.values/svgFiles/orphanedFiles to use p.basename to
generate enumFilenames comparison.
josefinalliende
approved these changes
Jan 23, 2026
Collaborator
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
✅ LGTM!
Tried it in my iPhone and all screens look good 👍🏻
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
WnIconwidget withWnIconsenum for type-safe icon usageIcons.xxxwithWnIcon(WnIcons.xxx, ...)across codebaseassets/svgs/WnWarningBoxto acceptWnIconsinstead ofIconData_SettingsTileto useWnIconsfor consistencyTest plan
flutter analyze- no issuesflutter test- all tests passdart format- all files formattedCloses #14
🤖 Generated with Claude Code
Summary by CodeRabbit
Style
UI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.