Add progress bars for categories and goals #4371
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
tbuist
force-pushed
the
tbuist-category-ui-bars
branch
3 times, most recently
from
70941bd to
b805cac
Compare
|
Do you have to do something to turn on the progress bars? What will happen when viewing multiple months? |
Yes, I added a global preference in the Themes section. I saw the documentation recommends against adding configuration for small UI tweaks, so I'm happy to hear feedback on if this is big enough to warrant a config option, or if it belongs somewhere else like a feature flag (or on by default, but I didn't want to presume).
The bars only appear in the sidebar that hosts the category titles, and only consider budgetting/spending in first month that is being looked at. Any thoughts about that? Two examples: |
Tick box on the settings page by the theme selector. Multi month looks like this, with the bars calculated for the left most month: Multi-month presents a tricky UI challenge, probably simplest might be to have it only calculated for the current month, and all hidden when the current month isn't on screen. My initial thoughts, were the bars look cramped. Categories without any goal, budgeted, or spent, still have the bar space reserved. Not sure about the mixed use of a bar to represent goal progress and budgeted vs spent as it's budgeting with goals, and usually goals are paired with templates, but the bar doesn't incorporate those because that way lies madness. I can see a category bar will go red because an expense was recorded against it, indicator a 'negative' or bad thing, when really it's fine because the category balance is still positive, and the money for it had been budgeted in a prior month. So a little misleading? Might be worth taking a look at #3424 which attempted to incorporate the multi-month aspects. |
This seems reasonable and intuitive to me.
Currently the color would be empty at least, so the bar still shows but doesn't draw as much attention.
Fair point.
Also a fair point.
Wow, I'm not sure how I missed that entire thread when looking for other attempts at this problem. After skimming, this does seem like a more complex attempt than what I intended mine to be. Thinking more deeply about what I envisioned these bars representing, I think I was targeting only the two most basic use cases:
I think those cover the intended use cases and avoid entirely the complexity of communicating single- and multi-month statuses for a category at the same time, but obviously does so by limiting the amount of information being shown. That might be a philosophical difference, but I will mull on what multi-month support would need to look like. |
|
I think the progress bars really should be per month and not next to the categories, if we are going to add them. Also this currently doesn't have any indication of underfunded categories which will need added too. |
|
I like the idea, but I find it too distracting, not sure if the colors are too intense, or the colors doesn't fit to the overall theme. also, I agree with @youngcw.
|
|
Here's a version that moves the bars out of the category column and also deemphasizes them a little bit by just shrinking the height - thoughts on this? Here's the same height in their original location My thought is that the shorter the horizontal length of the bar, the quicker I'm able to glean what the state of the category is without parsing the actual amounts. *Edit: |
|
The smaller height makes it way better! what if the current month shows the bar, but for other months you have to hover the month container? |
|
How do we feel about this? I'm just hooking into the same hover styling behavior that already exists on ExpenseCategoryMonth. Alternatively, I can explore highlighting every category in the month when any part of the month component is hovered. I didn't see a clean way to do this with the way the components are structured, but I can mess around some more if we want to see that. Separately, I've switched to using the report colors. |
|
One more to consider: |
|
This would be really nice to have! Thanks for the proposal, but I would prefer it how another tool (ynab) does it: There is a container, so the bar doesn't really go edge to edge, this would also simplify the view for multiple months so the bars do not touch. The coloring is very simple too, only 3 colors: green, striked green, striked red. Where striked means spent. This accounts for all the possibilities:
This will simplify with less colors and people coming from the other tool will feel more at home. |
|
I'm not sure on the hover thing. If you do add it, I think the hover to show all categories option (your second option) makes the most sense. For starters you could just leave the progress bars on for all months that have goal data. Then we could see if its distracting to have non-current month progress bars. I do appreciate the thin lines. Its not too obtrusive. I always thought the YNAB progress bars were way too big and made the budget table super cluttered. Back when I used YNAB I disabled the progress bars the second they got added to the app because they were annoying. The way you have the PR, I might actually try it. 🙂 It probably would be best to move the enable option into the "Category" menu. Same place as the "Toggle Hidden Categories" option. Im not sold on having progress bars on categories without goals. I feel like that detracts from the usefulness of the categories with templates if all categories have a progress bar looking for attention. |
Something like this? This made me realize that the non-goal progress bars are not of much use for non-current month visuals. They are most useful for communicating how much you have left to spend (although I suppose we still need to confirm whether this should be relative to the budgeted amount or the category balance - currently it's the budgeted amount), but 1) you can't change spending decisions for past months, and 2) spending for future months is always 0, even with scheduled transactions, so these would always be green under the current behavior.
Fewer colors helps.
Good idea, I'll move it here. |
|
I like how the other months are faded. how does it look like if the progress bar is just under the balance column? |
|
Here's just under the balance column. If we went this route, do you think the bar ratios would need to reflect spending relative to the balance, as opposed to spending relative to budgeted? I do think this is easier to read though.
|
I like this version. If its under the balance, I would prefer the progress bar to be the opposite. Right now you are showing from 100% to 0% (showing how much budgeted you still have based on spent). Could you update your branch with the current code so we can play with it too? |
|
Yep, just pushed everything. I moved the bar under the 'budgeted' column since that does make more sense with the current logic, like you mentioned. Let me know what you think. |
|
@tbuist , I played a little with your code, I liked it this way:
what do you think? Check my changes here: https://github.com/tbuist/actual/compare/tbuist-category-ui-bars...lelemm:actual:bars?expand=1 |
I really like how it looks! But how would a category not yet budgeted look like? If it is the bar background color it would look the same as budgeted=spent, which is not too bad but could make it harder to spot non budgeted categories, which should be one of the advantages of having visual cues. We can analyze the different scenarios: Good
Bad
I think all of this scenarios should be visually obvious via a quick glance. This should be possible to achieve by having the spent within budget as a shade of green, and the no spent part would be green (like the one you already used). The overspent part would be red. Background color would be gray (like the one you already used) which would represent no activity, no budget no spending. A combination of these would cover all scenarios. Opinions on the color palette may vary, but I think we need to start with an overview of all scenarios first. Speaking of which, Goals: Goals are more complicated because they are not really final on the current implementation but it would look something like this Good Bad Goals would need more thinking, but currently on a train and my stop is soon. |
|
Firstly, this is looking great excellent job!! But just a note and it maybe something that could be clarified in the docs. To me without initially making the connection that the progress bars are designed to visualize what's been budgeted and not the category balance throw me off. After looking at it more it made sense what it was showing but at first glance was a bit confusing. If it was requested to be based on budgeted amount rather than balance then I admit I have not read the whole thread. Just wanted to provide a potentially more outside perspective. |
would this have the same confusion where |
not sure if I understood what you mean. |
|
No matter which side you choose there will be templates that are based on the other column. So it seems like if you only use one column of space for the progress it will cause confusion. No matter which side is picked. |
what if we split the progress bar into two? do you think its still confusing? the current implementation of @tbuist it tries to show goals and budgeted all at the same progress bar.
|
FWIW, to me, this approach seems less confusing overall. Just a thought on adding progress bars for goals, would it make more sense to move them to the existing goal tooltip? |
My thought would be that in both cases (budgeted vs. spent, as well as goal progress), the budgeted amount is a component of the thing we're trying to communicate. That is, in both cases, I am interested in the budgeted amount for how much is remaining or for the impact it's having on the goal. I'm not sure I like multiple bars in one line.
I do like the narrowed width bars that you have here. |
tbuist
force-pushed
the
tbuist-category-ui-bars
branch
from
76d3dc8 to
409d30e
Compare
|
Super confusing, the bars should simplify not complicate reading the table, one bar per category should be enough. |
|
I settled on the following after messing around with a few of the suggestions in this thread. I'll submit it for review. |
tbuist
changed the title
WalkthroughThis pull request introduces several enhancements to the budgeting module, focusing on user preferences for displaying progress bars. A new state variable Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (2)
148-148: Consider using undefined instead of empty string for hover state reset.When setting the hovered month to "none" in the
onMouseOuthandler, you're using an empty string, but since the state was likely initialized asuseState<string | undefined>(), it might be more consistent to useundefinedinstead.- onMouseOut={() => setHoveredMonth('')} + onMouseOut={() => setHoveredMonth(undefined)}Also applies to: 159-160
241-242: Good implementation of hover state management.The use of
useLocalPreffor the progress bars preference and the mouse event handlers for hover state management are implemented appropriately.Same suggestion about using
undefinedinstead of empty string in theonMouseOuthandler applies here.- onMouseOut={() => setHoveredMonth('')} + onMouseOut={() => setHoveredMonth(undefined)}Also applies to: 260-261
packages/desktop-client/src/components/budget/ProgressBar.tsx (3)
184-195: Extract magic numbers as constants and simplify opacity logic.The opacity values and bar height are magic numbers that should be extracted as constants for better maintainability.
+const DEFAULT_OPACITY = '0.5'; +const FULL_OPACITY = '1'; +const BAR_HEIGHT = 3; +const BORDER_RADIUS = 30; + -const barHeight = 3; -const borderRadius = 30; -let barOpacity = '0.5'; // By default, all categories in all months with some activity are partly visible -if (isCurrentMonth) { - barOpacity = '1'; // By default, categories in the current month are fully visible -} -if (isCurrentMonth && hoveredMonth && hoveredMonth !== month) { - barOpacity = '0.5'; // If a non-current month is hovered over, lower visibility for the current month -} else if (hoveredMonth === month) { - barOpacity = '1'; // If a non-current month is hovered over, raise that month to fully visible -} +// Determine opacity based on current month and hover state +const barOpacity = + (hoveredMonth === month || (isCurrentMonth && (!hoveredMonth || hoveredMonth === month))) + ? FULL_OPACITY + : DEFAULT_OPACITY;
212-224: Use extracted constants for consistent styling.Update the style properties to use the constants we defined earlier.
<View style={{ - height: barHeight, + height: BAR_HEIGHT, backgroundColor: leftBar.color, width: `${leftBar.width}%`, position: 'absolute', bottom: 0, left: 0, - borderTopLeftRadius: borderRadius, - borderBottomLeftRadius: borderRadius, + borderTopLeftRadius: BORDER_RADIUS, + borderBottomLeftRadius: BORDER_RADIUS, transition: 'width 0.5s ease-in-out', }} title={`${t(leftBar.category)}: ${leftBar.rawValue}`} />Similarly update the right side bar component at lines 226-239.
43-116: Add more comments to clarify the complex color bar logic.The
getColorBarsfunction contains complex logic for different scenarios. While the current comments help, adding more detailed explanations, especially for the calculation formulas, would improve maintainability.For example:
if (isLongGoal) { // We have a long-term #goal set. These take visual precedence over a monthly template goal, even if both exist if (balance < 0) { - // Standard goal with a non-negative balance + // Standard goal with a negative balance - we're overspent relative to the goal + // Calculate total amount needed to reach goal from current negative position const toGoal = -1 * balance + goal; + // Calculate what percentage of the total goal amount we need to show as "remaining" leftBar.width = Math.min(Math.round((goal / toGoal) * 100), 100); rightBar.width = 100 - leftBar.width;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4371.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/budget/BudgetTable.tsx(3 hunks)packages/desktop-client/src/components/budget/BudgetTotals.tsx(3 hunks)packages/desktop-client/src/components/budget/ProgressBar.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx(7 hunks)packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetContext.tsx(4 hunks)packages/desktop-client/src/components/budget/index.tsx(2 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-11-10T16:45:25.627Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
🔇 Additional comments (16)
packages/loot-core/src/types/prefs.d.ts (1)
77-77: Type definition looks good!Adding the
'budget.showProgressBars'boolean preference to theLocalPrefstype is clear and follows the established pattern for local preferences in the application.packages/desktop-client/src/components/budget/BudgetTable.tsx (3)
88-90: Well-implemented local preference hook!The use of
useLocalPreffor managing the progress bars visibility preference is appropriate and follows the established pattern in the codebase.
229-231: Good implementation of toggle function.Simple and effective implementation of the toggle function for the progress bars visibility preference.
281-281: Properly passing the toggle function as prop.The
toggleProgressBarsfunction is correctly passed to theBudgetTotalscomponent, which will likely use it to provide a UI control for the user.packages/desktop-client/src/components/budget/index.tsx (2)
87-87: Good addition of hover state.Adding the
hoveredMonthstate is a good approach for enhancing the UI with hover effects.
366-367: Properly passing hover state to provider.The
hoveredMonthstate and setter are correctly passed to theEnvelopeBudgetProvidercomponent, enabling components downstream to access and update this state.packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (2)
26-26: Appropriate imports added.The imports for
useLocalPref,ProgressBar, anduseEnvelopeBudgetare added appropriately to support the new functionality.Also applies to: 36-36, 41-41
398-398:✅ Verification successful
Good conditional rendering of the progress bar.
The progress bar is conditionally rendered based on the user preference, which is the right approach. Make sure the
ProgressBarcomponent receives all the props it needs to render correctly.Let's check if the
ProgressBarcomponent exists and what props it expects:
🏁 Script executed:
#!/bin/bash # Look for the ProgressBar component definition fd -e tsx -e jsx "ProgressBar.tsx" --exec cat {} \;Length of output: 7593
Conditional rendering implemented correctly. The
ProgressBarcomponent exists and its prop interface expects onlymonth(astring) andcategory(aCategoryEntity), which are properly provided here. No additional props are required.packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetContext.tsx (3)
10-11: LGTM! Good addition of hoveredMonth state to the context.The new context state additions for tracking hovered month look good. The type definitions are clear and well-integrated with the existing code structure.
25-28: Consistent implementation of uninitialized context error handling.The error handling for uninitialized context methods follows the existing pattern in the codebase, which is good for consistency.
42-43: LGTM! Props are correctly passed through to the context.The implementation correctly accepts the new props and passes them to the context provider value.
Also applies to: 54-55
packages/desktop-client/src/components/budget/ProgressBar.tsx (1)
16-22: Nice use of theme colors for the progress bar.Using the theme colors provides consistent styling and will make it easier to maintain if theme colors change.
packages/desktop-client/src/components/budget/BudgetTotals.tsx (4)
21-21: LGTM! Good addition to the props type.The new prop for toggling progress bars is clearly defined in the type interface.
29-29: LGTM! Props are correctly destructured.The implementation correctly destructures the new prop.
90-92: LGTM! Appropriate handler for the new menu action.The new action handler for toggling progress bars follows the existing pattern.
104-107: LGTM! Menu item for toggling progress bars.The new menu item is correctly implemented with appropriate text using the translation function.
| useEffect(() => { | ||
| const setColorBars = async () => { | ||
| setTimeout(() => { | ||
| // Don't show visuals for income categories | ||
| if (category.is_income) { | ||
| return null; | ||
| } | ||
| const [freshLeftBar, freshRightBar] = getColorBars( | ||
| budgeted, | ||
| spent, | ||
| balance, | ||
| goal, | ||
| isLongGoal, | ||
| ); | ||
| setLeftBar(freshLeftBar); | ||
| setRightBar(freshRightBar); | ||
| }, 100); | ||
| }; | ||
|
|
||
| setColorBars(); | ||
| }, [category, budgeted, spent, balance, goal, isLongGoal, hoveredMonth]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary setTimeout and fix dependency array.
The useEffect contains a setTimeout that doesn't seem to serve a necessary purpose and could cause delayed rendering or UI flickering.
useEffect(() => {
- const setColorBars = async () => {
- setTimeout(() => {
// Don't show visuals for income categories
if (category.is_income) {
- return null;
+ return;
}
const [freshLeftBar, freshRightBar] = getColorBars(
budgeted,
spent,
balance,
goal,
isLongGoal,
);
setLeftBar(freshLeftBar);
setRightBar(freshRightBar);
- }, 100);
- };
-
- setColorBars();
-}, [category, budgeted, spent, balance, goal, isLongGoal, hoveredMonth]);
+}, [category, budgeted, spent, balance, goal, isLongGoal]);Note: hoveredMonth is in the dependency array but isn't used within the effect. Remove it unless you intend to use it for calculations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const setColorBars = async () => { | |
| setTimeout(() => { | |
| // Don't show visuals for income categories | |
| if (category.is_income) { | |
| return null; | |
| } | |
| const [freshLeftBar, freshRightBar] = getColorBars( | |
| budgeted, | |
| spent, | |
| balance, | |
| goal, | |
| isLongGoal, | |
| ); | |
| setLeftBar(freshLeftBar); | |
| setRightBar(freshRightBar); | |
| }, 100); | |
| }; | |
| setColorBars(); | |
| }, [category, budgeted, spent, balance, goal, isLongGoal, hoveredMonth]); | |
| useEffect(() => { | |
| // Don't show visuals for income categories | |
| if (category.is_income) { | |
| return; | |
| } | |
| const [freshLeftBar, freshRightBar] = getColorBars( | |
| budgeted, | |
| spent, | |
| balance, | |
| goal, | |
| isLongGoal, | |
| ); | |
| setLeftBar(freshLeftBar); | |
| setRightBar(freshRightBar); | |
| }, [category, budgeted, spent, balance, goal, isLongGoal]); |
|
The bars feel short. I think they should either be the full length of the cell input box, or the full table width (budgeted through to balance) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/desktop-client/src/components/budget/ProgressBar.tsx (8)
10-10: Use relative path import for loot-core types.The import for CategoryEntity uses an absolute path with multiple parent directory references which is harder to maintain. Use a relative path import instead for consistency with other imports.
-import { type CategoryEntity } from '../../../../loot-core/src/types/models'; +import { type CategoryEntity } from 'loot-core/src/types/models';
16-22: Extract colors to a theme constant object for better organization.The color definition constants should be organized within a single theme-related object for better maintainability and clarity.
-const ColorDefUnderBudgetRemaining = theme.reportsGreen; -const ColorDefUnderBudgetSpent = theme.reportsGray; -const ColorDefOverBudgetSpent = theme.reportsGray; -const ColorDefOverBudgetOverSpent = theme.reportsRed; -const ColorDefGoalRemaining = theme.reportsLabel; -const ColorDefGoalSaved = theme.reportsBlue; -const ColorDefEmpty = ''; // No color for default +const progressBarColors = { + underBudgetRemaining: theme.reportsGreen, + underBudgetSpent: theme.reportsGray, + overBudgetSpent: theme.reportsGray, + overBudgetOverSpent: theme.reportsRed, + goalRemaining: theme.reportsLabel, + goalSaved: theme.reportsBlue, + empty: '' // No color for default +};
178-191: Extract opacity calculation logic into a separate function.The opacity calculation logic is complex and would be more maintainable as a separate function.
+const getBarOpacity = (isCurrentMonth: boolean, hoveredMonth: string | null, month: string): string => { + if (isCurrentMonth && hoveredMonth && hoveredMonth !== month) { + return '0.5'; // If a non-current month is hovered over, lower visibility for the current month + } else if (hoveredMonth === month) { + return '1'; // If a non-current month is hovered over, raise that month to fully visible + } + + // By default, all categories in all months with some activity are partly visible + return isCurrentMonth ? '1' : '0.5'; +}; const BAR_HEIGHT = 3; const BORDER_RADIUS = 30; -const PARTIAL_OPACITY = '0.5'; -const FULL_OPACITY = '1'; -let barOpacity = PARTIAL_OPACITY; // By default, all categories in all months with some activity are partly visible -if (isCurrentMonth) { - barOpacity = FULL_OPACITY; // By default, categories in the current month are fully visible -} -if (isCurrentMonth && hoveredMonth && hoveredMonth !== month) { - barOpacity = PARTIAL_OPACITY; // If a non-current month is hovered over, lower visibility for the current month -} else if (hoveredMonth === month) { - barOpacity = FULL_OPACITY; // If a non-current month is hovered over, raise that month to fully visible -} +const barOpacity = getBarOpacity(isCurrentMonth, hoveredMonth, month);
193-236: Improve JSX structure and extract styles for better maintainability.The component has inline styles that could be extracted for better readability and maintainability.
Consider extracting styles:
+const containerStyle = { + display: 'flex', + position: 'absolute' as 'absolute', + right: 0, + bottom: 0, + marginBottom: 1, + width: '100%', + transition: 'opacity 0.25s', +}; + +const getBarStyle = (side: 'left' | 'right', color: string, width: string, height: number, borderRadius: number) => ({ + height, + backgroundColor: color, + width, + position: 'absolute' as 'absolute', + bottom: 0, + [side]: 0, + [`borderTop${side.charAt(0).toUpperCase() + side.slice(1)}Radius`]: borderRadius, + [`borderBottom${side.charAt(0).toUpperCase() + side.slice(1)}Radius`]: borderRadius, + transition: 'width 0.5s ease-in-out', +}); return ( <View style={{ - display: 'flex', - position: 'absolute', - right: 0, - bottom: 0, - marginBottom: 1, - width: '100%', + ...containerStyle, opacity: barOpacity, - transition: 'opacity 0.25s', }} > {/* Left side of the bar */} <View - style={{ - height: BAR_HEIGHT, - backgroundColor: leftBar.color, - width: `${leftBar.width}%`, - position: 'absolute', - bottom: 0, - left: 0, - borderTopLeftRadius: BORDER_RADIUS, - borderBottomLeftRadius: BORDER_RADIUS, - transition: 'width 0.5s ease-in-out', - }} + style={getBarStyle('left', leftBar.color, `${leftBar.width}%`, BAR_HEIGHT, BORDER_RADIUS)} title={`${t(leftBar.category)}: ${leftBar.rawValue}`} /> {/* Right side of the bar */} <View - style={{ - height: BAR_HEIGHT, - backgroundColor: rightBar.color, - width: `${rightBar.width}%`, - position: 'absolute', - bottom: 0, - right: 0, - borderTopRightRadius: BORDER_RADIUS, - borderBottomRightRadius: BORDER_RADIUS, - transition: 'width 0.5s ease-in-out', - }} + style={getBarStyle('right', rightBar.color, `${rightBar.width}%`, BAR_HEIGHT, BORDER_RADIUS)} title={`${t(rightBar.category)}: ${rightBar.rawValue}`} /> </View> );
219-219: Ensure tooltip text is accessible.The tooltips on the progress bars use raw values without proper formatting or screen reader context, which may impact accessibility.
Enhance the tooltip text to be more descriptive and accessible:
-title={`${t(leftBar.category)}: ${leftBar.rawValue}`} +title={`${category.name}: ${t(leftBar.category)} ${leftBar.rawValue}`} -title={`${t(rightBar.category)}: ${rightBar.rawValue}`} +title={`${category.name}: ${t(rightBar.category)} ${rightBar.rawValue}`}Also applies to: 234-234
154-154: Change Boolean constructor to boolean type casting.Using the Boolean constructor is less idiomatic in TypeScript than direct boolean casting with the double negation operator.
-const isLongGoal = Boolean(longGoal && longGoal > 0); +const isLongGoal = !!(longGoal && longGoal > 0);
43-116: Refactor getColorBars function to reduce code duplication.The
getColorBarsfunction has several blocks with similar patterns that could be refactored to reduce duplication and improve maintainability.Consider using a strategy pattern or case-based approach:
function getColorBars( budgeted: number, spent: number, balance: number, goal: number, isLongGoal: boolean, ) { const leftBar = new ColorBar(); const rightBar = new ColorBar(); // Define configurations for different scenarios const scenarios = { longGoalNegativeBalance: () => { const toGoal = -1 * balance + goal; return { left: { width: Math.min(Math.round((goal / (toGoal || 1)) * 100), 100), color: ColorDefGoalRemaining, rawValue: integerToCurrency(toGoal), category: 'Remaining' }, right: { width: null, // Will be calculated as 100 - leftWidth color: ColorDefOverBudgetOverSpent, rawValue: integerToCurrency(balance), category: 'Overspent' } }; }, longGoalPositiveBalance: () => { return { left: { width: Math.min(Math.round((balance / (goal || 1)) * 100), 100), color: ColorDefGoalSaved, rawValue: integerToCurrency(balance), category: 'Saved' }, right: { width: null, color: ColorDefGoalRemaining, rawValue: integerToCurrency(goal - balance), category: 'Remaining' } }; }, // Add other scenarios similarly }; // Select the appropriate scenario let scenario; if (isLongGoal) { scenario = balance < 0 ? scenarios.longGoalNegativeBalance() : scenarios.longGoalPositiveBalance(); } else if (spent * -1 >= budgeted) { scenario = scenarios.overBudget(); } else { scenario = scenarios.underBudget(); } // Apply the scenario Object.assign(leftBar, scenario.left); Object.assign(rightBar, { ...scenario.right, width: 100 - leftBar.width }); return [leftBar, rightBar]; }This approach makes adding new scenarios easier and reduces the repetitive width, color, and category assignments.
56-56: Improve code comments for better clarity.The comments for the long-term goal scenarios are identical despite describing different conditions.
-// Standard goal with a non-negative balance +// Long-term goal with a negative balance (overspent) -// Standard goal with a non-negative balance +// Long-term goal with a positive balance (making progress)Also applies to: 70-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/budget/ProgressBar.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
🔇 Additional comments (1)
packages/desktop-client/src/components/budget/ProgressBar.tsx (1)
162-176: Remove hoveredMonth from dependency array.The
hoveredMonthis in the useEffect dependency array but isn't used within the effect. This was previously identified in a review comment.useEffect(() => { // Don't show visuals for income categories if (category.is_income) { return; } const [freshLeftBar, freshRightBar] = getColorBars( budgeted, spent, balance, goal, isLongGoal, ); setLeftBar(freshLeftBar); setRightBar(freshRightBar); -}, [category, budgeted, spent, balance, goal, isLongGoal]); +}, [category, budgeted, spent, balance, goal, isLongGoal]);
| // Standard goal with a non-negative balance | ||
| const toGoal = -1 * balance + goal; | ||
| leftBar.width = Math.min(Math.round((goal / toGoal) * 100), 100); | ||
| rightBar.width = 100 - leftBar.width; |
There was a problem hiding this comment.
Fix calculation edge cases to prevent NaN or division by zero.
Several calculations in the component could result in NaN or division by zero errors when inputs are zero.
Add safeguards to prevent division by zero:
// Line 60 (and similar lines)
-leftBar.width = Math.min(Math.round((goal / toGoal) * 100), 100);
+leftBar.width = toGoal === 0 ? 100 : Math.min(Math.round((goal / toGoal) * 100), 100);
// Line 72
-leftBar.width = Math.min(Math.round((balance / goal) * 100), 100);
+leftBar.width = goal === 0 ? 0 : Math.min(Math.round((balance / goal) * 100), 100);
// Line 87-89
const overage = -1 * spent - budgeted;
const total = budgeted + overage;
-leftBar.width = Math.round((budgeted / total) * 100);
+leftBar.width = total === 0 ? 0 : Math.round((budgeted / total) * 100);
// Line 101-103
const remaining = budgeted - -1 * spent;
-leftBar.width = Math.round((remaining / budgeted) * 100);
+leftBar.width = budgeted === 0 ? 0 : Math.round((remaining / budgeted) * 100);Also applies to: 72-72, 87-89, 101-103
| @@ -0,0 +1,238 @@ | |||
| import { useState, useEffect } from 'react'; | |||
| import { useTranslation } from 'react-i18next'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use translations for category labels consistently.
The component imports and uses the translation hook for the tooltip titles, but the category labels in the getColorBars function are hardcoded in English.
Update the getColorBars function to accept the translation function and use it for category labels:
function getColorBars(
budgeted: number,
spent: number,
balance: number,
goal: number,
isLongGoal: boolean,
+ t: (key: string) => string,
) {
// ... existing code ...
// Instead of:
leftBar.category = 'Remaining';
// Use:
+ leftBar.category = t('Remaining');
// Apply this pattern to all category assignments in the function
}
// And in the component:
const [freshLeftBar, freshRightBar] = getColorBars(
budgeted,
spent,
balance,
goal,
isLongGoal,
+ t,
);Also applies to: 124-124
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/desktop-client/src/components/budget/ProgressBar.tsx (4)
58-69:⚠️ Potential issueFix calculation edge cases to prevent potential division by zero.
There are several calculations in the component that could result in division by zero errors or render NaN values:
Apply these safeguards to prevent division by zero:
- if (toGoal <= 0) { + if (goal === 0 || toGoal <= 0) { // If over the goal, consider it complete leftBar.width = 100; } else if (balance < 0) { // If balance is < 0, show no progress leftBar.width = 0; } else { // Otherwise, standard ratio with a positive balance and a positive amount to reach the goal - leftBar.width = bound(Math.round((balance / goal) * 100), 0, 100); + leftBar.width = goal === 0 ? 0 : bound(Math.round((balance / goal) * 100), 0, 100); }
78-79: 🛠️ Refactor suggestionUse translations for category labels consistently.
The component uses the translation hook for tooltip titles, but the category labels in the
getColorBarsfunction are hardcoded in English.Update the
getColorBarsfunction to accept the translation function and use it for category labels:function getColorBars( budgeted: number, spent: number, balance: number, goal: number, isLongGoal: boolean, + t: (key: string) => string, ) { // ...existing code... // Then update all labels with translation function - leftBar.category = 'Saved'; - rightBar.category = 'Remaining'; + leftBar.category = t('Saved'); + rightBar.category = t('Remaining'); // And similarly for other labels } // And in the component: const [freshLeftBar, freshRightBar] = getColorBars( budgeted, spent, balance, goal, isLongGoal, + t, );Also applies to: 93-94, 107-108
97-99:⚠️ Potential issueProtect against division by zero.
Another potential division by zero when calculating under budget scenarios:
const remaining = budgeted + spent; -leftBar.width = bound(Math.round((remaining / budgeted) * 100), 0, 100); +leftBar.width = budgeted === 0 ? 0 : bound(Math.round((remaining / budgeted) * 100), 0, 100);
82-85:⚠️ Potential issueAdd safeguards against division by zero in calculations.
Similar division by zero issue exists in this section:
const overage = budgeted + spent; const total = budgeted + Math.abs(overage); -leftBar.width = bound(Math.round((budgeted / total) * 100), 0, 100); +leftBar.width = total === 0 ? 0 : bound(Math.round((budgeted / total) * 100), 0, 100);
🧹 Nitpick comments (3)
packages/desktop-client/src/components/budget/ProgressBar.tsx (3)
178-181: Extract magic numbers to theme constants.The component defines magic numbers for styling, which would be better placed in the theme.
Consider moving these styling constants to your theme file for consistency and easier maintenance:
// In your theme file + export const progressBar = { + height: 3, + borderRadius: 30, + partialOpacity: '0.4', + fullOpacity: '1', + transitionDuration: '0.25s', + widthTransition: 'width 0.5s ease-in-out', + }; // Then in your component - const BAR_HEIGHT = 3; - const BORDER_RADIUS = 30; - const PARTIAL_OPACITY = '0.4'; - const FULL_OPACITY = '1'; + const { height: BAR_HEIGHT, borderRadius: BORDER_RADIUS, partialOpacity: PARTIAL_OPACITY, fullOpacity: FULL_OPACITY } = theme.progressBar;
196-204: Extract the styling into a separate style object.All component styles are defined inline, which makes the JSX harder to read.
Consider extracting styles into separate objects:
+const styles = { + container: { + display: 'flex', + position: 'absolute' as const, + right: 0, + bottom: 0, + marginBottom: 1, + width: '100%', + opacity: barOpacity, + transition: 'opacity 0.25s', + }, + leftBar: (width: number) => ({ + height: BAR_HEIGHT, + backgroundColor: leftBar.color, + width: `${width}%`, + position: 'absolute' as const, + bottom: 0, + left: 0, + borderTopLeftRadius: BORDER_RADIUS, + borderBottomLeftRadius: BORDER_RADIUS, + transition: 'width 0.5s ease-in-out', + }), + rightBar: (width: number) => ({ + height: BAR_HEIGHT, + backgroundColor: rightBar.color, + width: `${width}%`, + position: 'absolute' as const, + bottom: 0, + right: 0, + borderTopRightRadius: BORDER_RADIUS, + borderBottomRightRadius: BORDER_RADIUS, + transition: 'width 0.5s ease-in-out', + }), +}; return ( <View - style={{ - display: 'flex', - position: 'absolute', - right: 0, - bottom: 0, - marginBottom: 1, - width: '100%', - opacity: barOpacity, - transition: 'opacity 0.25s', - }} + style={styles.container} > {/* Left side of the bar */} <View - style={{...}} + style={styles.leftBar(leftBar.width)} title={`${t(leftBar.category)}: ${leftBar.rawValue}`} /> {/* Right side of the bar */} <View - style={{...}} + style={styles.rightBar(rightBar.width)} title={`${t(rightBar.category)}: ${rightBar.rawValue}`} /> </View> );
210-217: Improve accessibility with aria attributes.The progress bars should include appropriate ARIA attributes for accessibility.
<View style={{ height: BAR_HEIGHT, backgroundColor: leftBar.color, width: `${leftBar.width}%`, position: 'absolute', bottom: 0, left: 0, borderTopLeftRadius: BORDER_RADIUS, borderBottomLeftRadius: BORDER_RADIUS, transition: 'width 0.5s ease-in-out', }} title={`${t(leftBar.category)}: ${leftBar.rawValue}`} + role="progressbar" + aria-valuemin="0" + aria-valuemax="100" + aria-valuenow={leftBar.width} + aria-label={`${t(leftBar.category)}: ${leftBar.rawValue}`} />Apply the same changes to the right bar as well.
Also applies to: 225-232
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/budget/ProgressBar.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
🔇 Additional comments (1)
packages/desktop-client/src/components/budget/ProgressBar.tsx (1)
162-176: Remove unnecessary dependency in useEffect and optimize the implementation.The effect doesn't depend on
hoveredMonthbut it's included in the previous commit's dependency array (as noted in past review comments).useEffect(() => { // Don't show visuals for income categories if (category.is_income) { return; } const [freshLeftBar, freshRightBar] = getColorBars( budgeted, spent, balance, goal, isLongGoal, + t, ); setLeftBar(freshLeftBar); setRightBar(freshRightBar); }, [category, budgeted, spent, balance, goal, isLongGoal, t]);Note: I've added
tto the dependency array since we'll now use it in thegetColorBarsfunction.
| const overage = budgeted + spent; | ||
| const total = budgeted + Math.abs(overage); |
There was a problem hiding this comment.
Fix the overspent amount calculation logic.
The current calculation for overage when overspent doesn't match the intention in the comments:
// We overspent (or are exactly at budget)
-const overage = budgeted + spent;
+const overage = spent + budgeted; // spent is negative, so this is actually (spent*-1 - budgeted)Better yet, make the calculation more explicit:
// We overspent (or are exactly at budget)
-const overage = budgeted + spent;
+const overspent = -spent - budgeted; // Calculate how much was overspentCommittable suggestion skipped: line range outside the PR's diff.
| } else if (balance < 0) { | ||
| // If balance is < 0, show no progress | ||
| leftBar.width = 0; | ||
| } else { | ||
| // Otherwise, standard ratio with a positive balance and a positive amount to reach the goal | ||
| leftBar.width = bound(Math.round((balance / goal) * 100), 0, 100); | ||
| } |
There was a problem hiding this comment.
Add handling for edge case when goal is 0.
The current implementation doesn't properly handle the case when a goal is set to 0.
if (toGoal <= 0) {
// If over the goal, consider it complete
leftBar.width = 100;
+} else if (goal === 0) {
+ // If goal is 0, don't show progress
+ leftBar.width = 0;
} else if (balance < 0) {
// If balance is < 0, show no progress
leftBar.width = 0;Committable suggestion skipped: line range outside the PR's diff.
Fixes #2965
This change introduces visual bars to indicate the status of a particular category for that month.
For standard expense categories, green, red, and gray indicate the value you have remaining/overspent in that category.
For categories with a #goal template, the bar uses blue to indicate the progress.