Support icons - #261
Open
sonisumit06 wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Goal.icon API typing doesn’t match the stated “optional” contract and the GoalManager changes are currently incomplete/unused relative to the PR’s described icon-editing functionality.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds support for associating and displaying an icon (intended as an emoji) with Goals in the UI and API typings.
Changes:
- Extends the
Goaltype with aniconfield. - Renders the goal icon on the goal card UI.
- Adds dependencies/groundwork for emoji selection (emoji-mart) and begins wiring it into GoalManager (currently import-only in this diff).
File summaries
| File | Description |
|---|---|
| src/ui/pages/Main/goals/GoalCard.tsx | Displays the goal’s icon on the card (currently via Material UI Icon). |
| src/ui/features/goalmanager/GoalManager.tsx | Adds emoji/icon-related imports (no functional icon editing logic present in this diff). |
| src/api/types.ts | Adds icon to the Goal interface. |
| package-lock.json | Records metadata for the added emoji-mart dependency. |
Review details
Suppressed comments (2)
src/ui/pages/Main/goals/GoalCard.tsx:34
- Rendering the emoji inside Material UI’s
Iconcomponent is intended for Material Icons ligatures and may not display/stylize emoji as expected. Also, if the emoji is decorative, it should be hidden from screen readers; if it conveys meaning, it needs an accessible label. Consider rendering it as plain text and only when present.
<TargetAmount>${goal.targetAmount}</TargetAmount>
<TargetDate>{asLocaleDateString(goal.targetDate)}</TargetDate>
<Icon>{goal.icon}</Icon>
</Container>
src/ui/features/goalmanager/GoalManager.tsx:6
- The PR description says GoalManager was updated to allow adding/changing goal icons via an emoji picker, but in this file the added emoji-related imports aren’t used and there’s no state/UI/updateGoal logic for
goal.icon(only name/amount/date updates are present). Either complete the icon editing flow here or adjust the PR description/scope.
import { faSmile } from '@fortawesome/free-regular-svg-icons'
import { BaseEmoji } from 'emoji-mart'
import EmojiPicker from '../../components/EmojiPicker'
import { faCalendarAlt } from '@fortawesome/free-regular-svg-icons'
import { faDollarSign, IconDefinition } from '@fortawesome/free-solid-svg-icons'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
- Files reviewed: 3/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
20
to
24
| export interface Goal { | ||
| id: string | ||
| name: string | ||
| icon: string | null | ||
| targetAmount: number |
Comment on lines
+1
to
6
| import { faSmile } from '@fortawesome/free-regular-svg-icons' | ||
| import { BaseEmoji } from 'emoji-mart' | ||
| import EmojiPicker from '../../components/EmojiPicker' | ||
| import { faCalendarAlt } from '@fortawesome/free-regular-svg-icons' | ||
| import { faDollarSign, IconDefinition } from '@fortawesome/free-solid-svg-icons' | ||
| import { FontAwesomeIcon } from '@fortawesome/react-fontawesome' |
Comment on lines
10
to
12
| import { Card } from '../../../components/Card' | ||
| import Icon from '@material-ui/core/Icon/Icon' | ||
|
|
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
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.
Changes
iconfield to the Goal model.Testing