[WIP] POC Budget Bar Graph #3424
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 (largest 100 files by percent change)
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
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
|
|
Correct me if I'm reading the colours wrong: Green = budgeted I think it's doing the maths wrong on this one, it appears to be tacking on the positive carry over to the end, instead of including it in the budgeted/spent part.
The green only being for the budgeted this month amount and for a big goal with a small amount added each month looks a bit odd because the budgeted amount is such a small proportion of the carried over balance:
Would it look better if the roll over were a different shade of green(for positive over), and be left aligned so it looks like it stacks with the this month budgeted? This would also emphasis the idea of how money carries over add you add to it each month. Excuse the bad paint edit: Within the spent colour the amount carried over could also be left aligned within the spent amount, possibly a lighter purple if you want to show the source of funds otherwise no change? I'm not really the target (hah!) for this but I think it's highly requested. I wonder what people's thoughts would be on having a rare option for this and tie its visibility to the templates feature flag? Thinking three levels, None, On Hover, Always Showing. |
|
Budget Bar Graph is definitely something I have been looking forward to in Actual Budget! To add to @Teprifer 's comment, the three levels I would suggest is how Notion does table properties: Hidden Always, Hide if Empty, Display Always. I am asking in the Discord how to test edge builds because I only have Pikapods right now, but I was wondering how would the graph behave if a category has 5 months worth of money, and I want it to be kept with 5 months max and fluctuate between 4-5 months as I refill the margin? Also @Teprifer , I appreciate the pun 🤣 |
|
@Jonathan-Fang there's a netlify link near the top to try out this PR. I usually open it in an incognito window or another browser to have a clean environment. Then I export my budget from my regular install and import it into the netlify link, remember the app runs locally so you're not uploading your data anywhere. Depending on the PR I might try a test or fresh budget instead. |
|
It's not tied to templates, it's displaying budget vs spent so it's usable without any usage of templates. Also, I don't think that adding a setting for this is necessary. It's a discovered feature that doesn't hinder or change a user's experience of the budget table. It's only shown on hover and never shown when empty. I'd agree that colors need to be optimized. |
|
RE: Colors Here are all the possible outcomes (I can think of) - please excuse the rushed MS paint work...
|
|
So I have 2 questions:
|
|
I find this quite distracting - mainly because of the hover states. I prefer to have a simplistic UI, maybe we can make this opt-in by giving an option on the category/group? Something like Show progress From a user perspective I think the colors are not immediately obvious. My first thought was "What are purple, red, black and green supposed to mean". I think if the user has to ask that question it's too complicated.
My two cents for the No Rollover options
The user would have the option to turn it on for a category or group, and it would always be visible if on - no hover states. |
|
Doh, I mis-interpreted the description. I think a setting would still be a good idea as I can see some people wanting it on all the time(it'd look busy, but each to their own eh), as is, or not shown.
|
|
Big fan of this! I find myself trying to hover for a tooltip that explains what the bar segment means, if it's possible to add then it would alleviate any confusion on what the colours mean, and would stop people having to work it out for themselves Count me in as an advocate for a display setting, ideally in three states:
|
WalkthroughThe changes introduce a new state management system for displaying progress and controlling the visibility of hidden categories within the budget management application. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/ExpenseCategory.tsx (1)
78-78: LGTM!The addition of the
heightproperty to theRowcomponent aligns with the PR objective of introducing visual enhancements to the budget management application. Setting a fixed height ensures consistent spacing and alignment of expense categories, which can enhance the user experience.Please consider the following suggestions:
- Ensure that the fixed height of
44accommodates all necessary content within theRowcomponent and provides sufficient padding.- Test the responsiveness of the
Rowcomponent with the fixed height to ensure that it adapts well to different screen sizes and device orientations.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/desktop-client/src/components/budget/ExpenseCategory.tsx (1 hunks)
- packages/desktop-client/src/components/budget/report/ReportComponents.tsx (4 hunks)
- packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (4 hunks)
- packages/desktop-client/src/components/reports/graphs/BarGraphVertical.tsx (1 hunks)
- packages/desktop-client/src/style/themes/dark.ts (1 hunks)
- packages/desktop-client/src/style/themes/development.ts (1 hunks)
- packages/desktop-client/src/style/themes/light.ts (1 hunks)
- packages/desktop-client/src/style/themes/midnight.ts (1 hunks)
- upcoming-release-notes/3424.md (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/style/themes/light.ts
Additional comments not posted (13)
packages/desktop-client/src/components/reports/graphs/BarGraphVertical.tsx (5)
1-8: LGTM!The imports are correctly specified and follow the proper syntax. The imported dependencies are relevant to the functionality of the component.
9-16: LGTM!The
barGraphBudgetCategorytype definition correctly specifies the fields and their corresponding data types. Exporting the type definition makes it accessible to other modules if needed.
17-112: Skipping the commented-out code segment.The commented-out code suggests potential features that could be implemented later, such as a custom tooltip component. However, it does not affect the current functionality of the component.
113-116: LGTM!The
BarGraphVerticalPropstype definition correctly specifies the expected properties and their corresponding data types. Thestyleprop is optional, allowing flexibility in styling the component, while thedataprop is required and expects an array ofbarGraphBudgetCategoryobjects.
118-181: LGTM!The
BarGraphVerticalcomponent correctly utilizes therechartslibrary to render a vertical bar chart. The responsive container ensures that the chart adapts to its parent dimensions, while the stacked bar configuration effectively represents the budget data, with each category represented by multiple color-coded bars. The hiding ofXAxisandYAxislabels focuses the user's attention on the bars themselves. The component correctly handles thedataprop and renders the chart accordingly.packages/desktop-client/src/style/themes/development.ts (1)
193-193: LGTM!The new color constant
reportsPurpleis correctly added to the theme file. It follows the existing naming convention and expands the color options available for reporting purposes.packages/desktop-client/src/style/themes/dark.ts (1)
194-194: LGTM!The addition of the
reportsPurplecolor constant is a straightforward change that expands the color palette for reports. The naming convention is consistent, and the color value is sourced from the existingcolorPaletteobject.packages/desktop-client/src/style/themes/midnight.ts (1)
196-196: LGTM!The addition of the
reportsPurplecolor constant is consistent with the existing color constants used for reporting purposes. It follows the naming convention and uses a color value from thecolorPalettemodule.This change expands the color options available for reporting features in the application without introducing any breaking changes.
packages/desktop-client/src/components/budget/report/ReportComponents.tsx (1)
Line range hint
225-506: LGTM!The
CategoryMonthcomponent has been enhanced with hover functionality to display a tooltip with a bar graph, providing a better visual representation of the budget data. The implementation looks solid with the following highlights:
- The hover state is correctly managed using
useCallbackfor pointer events and a timeout to delay the hover effect.- The
useEffecthook is used to reset the hover state whenever the component updates, ensuring consistency.- The budget data fetching logic using
useRolloverSheetValueand the structuring of thedataarray for the bar graph is implemented correctly.- The
BarGraphVerticalcomponent is conditionally rendered based on theisHoveredstate, which is the expected behavior.- The component is using memoization with
memo, which can help optimize performance by avoiding unnecessary re-renders.Great job with the enhancements!
packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (4)
211-234: Hover state management logic looks good!The
isHoveredstate variable andhandlePointerEnter,handlePointerLeavefunctions effectively manage the hover state. The use of a timeout to delay the hover effect is a nice touch to prevent flickering on quick mouse movements. Forcing the tooltip to close whenever the component's disablement state changes ensures that the UI remains responsive.
236-260: Bar graph data calculation is comprehensive and accurate.The bar graph data is calculated using the category's budgeted amount (
catBudgeted), spent amount (catSumAmount), and balance (catBalance). Thebudget,spent,remaining,overBudget, andcarryovermetrics provide a comprehensive overview of the category's budget performance. The logic handles edge cases like overspending and negative balances correctly.
264-265: Addition of hover event handlers to the component's layout is correct.The
onMouseEnterandonMouseLeaveevent handlers are correctly added to theViewcomponent that wraps the category row. This ensures that the hover state is triggered when the user hovers over any part of the category row.
485-486: Rendering the bar graph component on hover is visually appealing and informative.The
BarGraphVerticalcomponent is rendered when theisHoveredstate is true. The styling ensures that the bar graph is positioned correctly below the category row and is visually aligned with the budget, spent, and balance columns. The flexGrow style allows the bar graph to expand to fill the available space.
|
Thanks @Teprifer for the tip on trying out the edge build with the deploy preview link. I'm finding all the possible permutations confusing and unintuitive at first glance, although I understand they are supposed to work with rollover. And perhaps the use-case I was looking for is far more simplistic than what is being suggested here.
I'm honestly not sure how to deal with rollover, but I think that 15 different combinations of colors is much. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/report/ReportComponents.tsx (1)
501-511: Consider extracting the bar graph rendering logic into a separate component.To improve code readability and reusability, consider extracting the bar graph rendering logic into a separate component. This will help keep the
CategoryMonthcomponent focused on its primary responsibility of rendering the category's budget, spent, and balance, while delegating the bar graph rendering to a dedicated component.Here's an example of how you can extract the bar graph rendering logic:
+const CategoryMonthBarGraph = ({ data, isHovered }: { data: barGraphBudgetCategory[], isHovered: boolean }) => { + return ( + <View + style={{ + height: 13, + marginTop: -5, + }} + > + {isHovered && ( + <BarGraphVertical style={{ flexGrow: 1 }} data={data} /> + )} + </View> + ); +}; + // ... return ( <View // ... > // ... {showProgress && ( - <View - style={{ - height: 13, - marginTop: -5, - }} - > - {isHovered && ( - <BarGraphVertical style={{ flexGrow: 1 }} data={data} /> - )} - </View> + <CategoryMonthBarGraph data={data} isHovered={isHovered} /> )} </View> );
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- packages/desktop-client/src/components/budget/BudgetTable.jsx (2 hunks)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (2 hunks)
- packages/desktop-client/src/components/budget/ExpenseCategory.tsx (3 hunks)
- packages/desktop-client/src/components/budget/report/ReportComponents.tsx (4 hunks)
- packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (4 hunks)
- packages/desktop-client/src/components/common/Menu.tsx (1 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/common/Menu.tsx
Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/budget/ExpenseCategory.tsx
- packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx
Additional comments not posted (9)
packages/loot-core/src/types/prefs.d.ts (1)
73-73: LGTM!The addition of the
'budget.showProgress': boolean;property to theLocalPrefstype is a valid change that expands the configuration options for budget-related preferences. It allows users to specify whether to display progress indicators in the budget section, providing more granular control over the user interface related to budget management.packages/desktop-client/src/components/budget/BudgetTotals.tsx (3)
26-29: LGTM!The
BudgetTotalscomponent has been updated to use the new props for managing the visibility of hidden categories and progress bars, which is consistent with the changes made to theBudgetTotalsPropstype.
86-93: Approve the updated menu selection handling.The menu selection handling has been updated to use the new props for managing the visibility of hidden categories and progress bars. This aligns with the changes made to the
BudgetTotalsPropstype and enhances the menu functionality by providing independent toggles for each feature.
104-111: LGTM!The menu items now include toggles for both
showHiddenCategoriesandshowProgressprops, allowing users to control these features directly from the menu. This enhances the user experience and is consistent with the updates made to the menu selection handling.packages/desktop-client/src/components/budget/BudgetTable.jsx (3)
44-46: LGTM!The introduction of the
showProgressstate variable using theuseLocalPrefhook is a good addition. It follows the existing pattern in the component and allows for persisting the user's preference for showing or hiding progress-related elements.
Line range hint
1-300: LGTM!The removal of the
onToggleHiddenCategoriesandtoggleHiddenCategoriesfunctions is a good change. It helps to keep the component focused on its primary responsibilities and improves the separation of concerns.As long as the responsibility of toggling the visibility of hidden categories is properly handled in the
BudgetTotalscomponent, this change should not have a negative impact on the functionality of the component.
200-203: Verify the impact on theBudgetTotalscomponent.The
BudgetTotalscomponent is now receiving props related to managing the visibility of hidden categories and progress. This suggests a shift in the responsibility of managing these states from the parent component to the child component.Please ensure that the
BudgetTotalscomponent is updated to handle these new props correctly and that the overall logic and structure of the component remain cohesive.Run the following script to verify the usage of these props in the
BudgetTotalscomponent:Verification successful
Verification complete: Changes correctly implemented in BudgetTotals
The
BudgetTotalscomponent has been successfully updated to handle the new props related to managing the visibility of hidden categories and progress. The component is located inpackages/desktop-client/src/components/budget/BudgetTotals.tsxand includes the following new props:
setShowHiddenCategoriesPefshowHiddenCategoriessetShowProgressshowProgressThese props are correctly defined in the component's props type and are passed down from the
BudgetTablecomponent. The recent changes to the file, including its conversion to TypeScript and added translation support, indicate that the component is being actively maintained and improved.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new props in the `BudgetTotals` component. # Test: Search for the usage of the new props. Expect: Usage in the `BudgetTotals` component. rg --type jsx -A 5 $'setShowHiddenCategoriesPef|showHiddenCategories|setShowProgress|showProgress' packages/desktop-client/src/components/budget/BudgetTotals.jsxLength of output: 190
Script:
#!/bin/bash # Description: Locate BudgetTotals.jsx and search for new props usage # Step 1: Find the BudgetTotals.jsx file echo "Searching for BudgetTotals.jsx file:" fd BudgetTotals.jsx # Step 2: If the file is found, search for the new props echo -e "\nSearching for new props usage:" fd BudgetTotals.jsx --exec grep -n -A 5 'setShowHiddenCategoriesPef\|showHiddenCategories\|setShowProgress\|showProgress' {} \;Length of output: 314
Script:
#!/bin/bash # Description: Investigate the location and recent changes of BudgetTotals component # Step 1: Search for any file containing "BudgetTotals" in its name echo "Searching for files containing 'BudgetTotals' in the name:" fd BudgetTotals # Step 2: Search for "BudgetTotals" content in all files echo -e "\nSearching for 'BudgetTotals' content in all files:" rg "BudgetTotals" -n -C 2 # Step 3: Check git history for recent changes related to BudgetTotals echo -e "\nChecking git history for BudgetTotals changes:" git log -n 5 --pretty=format:"%h - %s" --grep="BudgetTotals"Length of output: 6770
packages/desktop-client/src/components/budget/report/ReportComponents.tsx (2)
227-250: Hover functionality implementation looks good!The hover functionality is implemented correctly using the
isHoveredstate variable andhoverTimeoutRefref. ThehandlePointerEnterandhandlePointerLeavecallbacks update the hover state with a timeout mechanism, providing a smooth user experience. TheuseEffecthook ensures that the hover state is reset whenever the component updates, preventing any stale state issues.
212-212: Good usage ofuseRolloverSheetValueanduseLocalPrefhooks.The
useRolloverSheetValuehook is used effectively to retrieve the necessary budget-related metrics (catSumAmount,catBudgeted, andcatBalance) for rendering the bar graph. Organizing the retrieved values into adataarray makes it convenient to pass the data to theBarGraphVerticalcomponent.The
useLocalPrefhook is used appropriately to manage the visibility of the progress indicators based on user preferences, providing flexibility in the UI.Also applies to: 252-276
|
@coderabbitai review |
Actions performedReview triggered.
|
|
I've added a menu option and a legend in the menu tooltip. I also, decreased permutations by about half. Only using 6 colors that follow an easy logic. (I hope - let me know) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (2 hunks)
Additional context used
GitHub Check: lint
packages/desktop-client/src/components/budget/BudgetTotals.tsx
[warning] 3-3:
There should be no empty line within import group
[warning] 151-151:
Delete··
Additional comments not posted (4)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (4)
10-10: LGTM!The import statement for the
Textcomponent is valid and necessary for the component.
16-44: Approve the newLegendcomponent.The introduction of the
Legendcomponent is a valuable addition to the codebase. It enhances the user interface by providing clear visual cues for different categories associated with progress bars. The component's implementation is straightforward and follows the common React component structure.The
Legendcomponent aligns well with the PR objectives of improving the user experience with visual legends.
48-51: The past review comment suggesting to fix the typo in the prop namesetShowHiddenCategoriesPefis still valid.Please apply this diff to fix the typo:
- setShowHiddenCategoriesPef: (value: boolean) => void; + setShowHiddenCategoriesPref: (value: boolean) => void;
Line range hint
65-172: Approve the updates to the menu selection handling and the use of theLegendcomponent.The updates to the menu selection handling align well with the modifications made to the
BudgetTotalsPropstype and the new props for managing the visibility of hidden categories and progress bars. The inclusion of toggles forshowHiddenCategoriesandshowProgressin the menu items allows users to control these features directly from the menu, enhancing the user experience.The use of the
Legendcomponent in the custom tooltip for theshowProgressmenu item is a great addition. It provides clear visual cues for different categories associated with progress bars, improving the user interface and aligning with the PR objectives.Tools
GitHub Check: lint
[warning] 151-151:
Delete··
Brave on Linux |
If it helps, it shows me for in Chrome and Firefox, windows 11. A nice addition, thanks :)
Yeah empty would probably be better than no bar for the 'is it working' aspect. |
I had brought up a separate request, and this is actually a perfect solution for it! The toggle allows the user to know when it's been turned on and turned off. Other than renaming the button as you change it.. ¯\_(ツ)_/¯ |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (1)
119-172: LGTM: Enhanced Menu functionality with minor suggestionThe changes to the Menu component are well-implemented and align with the PR objectives:
- The increased menu width accommodates the new items.
- New menu items for visibility management are correctly implemented.
- The custom tooltip with legends enhances user understanding of the progress bars feature.
Consider extracting the custom tooltip content into a separate component for better code organization and reusability. This would make the Menu items definition cleaner and easier to maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (2 hunks)
Additional comments not posted (5)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (5)
4-4: LGTM: New import for useSyncedPrefThe addition of the
useSyncedPrefhook import is appropriate for managing synchronized preferences, which aligns with the new visibility management functionality.
16-44: LGTM: New Legend componentThe
Legendcomponent is a well-structured and reusable addition that enhances the UI by providing visual cues for different categories. Its implementation is clean and follows good React practices.
48-51: LGTM: New props for visibility managementThe addition of
setShowHiddenCategories,showHiddenCategories,setShowProgress, andshowProgressprops enhances user control over the budget display, aligning well with the PR objectives. The prop names are clear and descriptive.
58-61: LGTM: Implementation of visibility managementThe
BudgetTotalscomponent correctly implements the new visibility management functionality using the added props. The use ofuseSyncedPreffor getting the budget type is appropriate and consistent with the new import.Also applies to: 65-65
Line range hint
1-191: Overall assessment: Well-implemented feature with good attention to user experienceThe changes in this file effectively implement the budget bar graph feature as described in the PR objectives. Key points:
- The new
Legendcomponent enhances visual understanding.- Visibility management for hidden categories and progress bars is well-implemented.
- The menu updates provide clear user controls for the new features.
- The custom tooltip with legends improves user comprehension of the progress bars.
The code is clean, well-structured, and aligns with React best practices. The only suggestion is to consider extracting the custom tooltip content into a separate component for better maintainability.
Great job on implementing this feature!
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (1)
119-172: LGTM: Enhanced menu with new visibility options and legend tooltipThe menu has been successfully updated with new items for controlling the visibility of hidden categories and progress bars. The custom tooltip with legends for the progress bars is a great addition that enhances user understanding.
Consider extracting the legend rendering logic into a separate function to improve code organization. This would make the
customTooltipprop more readable. For example:const renderLegends = () => ( <View style={{ padding: 10, paddingBottom: 0 }}> {budgetType === 'rollover' && ( <> <Legend title="Rollover added" color={theme.reportsLightPurple} /> <Legend title="Rollover spent" color={theme.reportsLightGreen} /> <Legend title="Rollover overspent" color={theme.reportsLightRed} /> </> )} <Legend title="Budgeted" color={theme.reportsPurple} /> <Legend title="Spent" color={theme.reportsGreen} /> <Legend title="Overspent" color={theme.reportsRed} /> </View> ); // Then in the menu items: customTooltip: renderLegends()This refactoring would improve the readability and maintainability of the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (2 hunks)
Additional comments not posted (4)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (4)
4-4: LGTM: New import for useSyncedPrefThe addition of the
useSyncedPrefhook import is appropriate for managing synchronized preferences, which aligns with the new visibility management features.
16-44: LGTM: New Legend component enhances UIThe new
Legendcomponent is a well-structured addition that enhances the UI by providing clear visual cues for different budget categories. This aligns well with the PR objectives of improving the visual representation of budget data.
48-51: LGTM: New props for visibility managementThe new props
setShowHiddenCategories,showHiddenCategories,setShowProgress, andshowProgressare well-named and align with the PR objectives of enhancing user control over budget visualization. The previous typo in the prop name has been correctly addressed.
58-61: LGTM: Updated component with new visibility management propsThe
BudgetTotalscomponent has been correctly updated to use the new visibility management props. The addition ofuseSyncedPreffor the budget type is a good improvement for maintaining consistent preferences across the application.Also applies to: 65-65
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (1)
Line range hint
1-624: Overall improvements with some optimization opportunitiesThe changes to the ExpenseCategoryMonth component significantly enhance the budget management interface by introducing progress display features and improved menu interactions. The new hover functionality and conditional rendering of the BarGraphVertical component provide users with more detailed budget information.
However, there are a few areas that could be further optimized:
- Consider memoizing complex calculations or components to improve performance, especially if this component is rendered frequently.
- The file is becoming quite large and complex. Consider splitting it into smaller, more focused components for better maintainability.
- Ensure that all new UI elements are fully accessible, including proper ARIA labels and keyboard navigation support.
Overall, the changes are a positive improvement to the budget management functionality.
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1)
234-244: Simplify hover state management by removing unnecessary timeoutIn the
handlePointerEnterfunction,setTimeoutis used with a delay of0, which effectively does not delay execution. This adds unnecessary complexity without any benefit. You can simplify the hover state management by removing thesetTimeoutand the associatedhoverTimeoutRef.Apply this diff to simplify the code:
- const [isHovered, setIsHover] = useState(false); - const hoverTimeoutRef = useRef<ReturnType<typeof setTimeout>>(); - const handlePointerEnter = useCallback(() => { - const timeout = setTimeout(() => { - setIsHover(true); - }, 0); - hoverTimeoutRef.current = timeout; - }, []); - const handlePointerLeave = useCallback(() => { - if (hoverTimeoutRef.current) { - clearTimeout(hoverTimeoutRef.current); - } - setIsHover(false); - }, []); + const [isHovered, setIsHovered] = useState(false); + const handlePointerEnter = useCallback(() => { + setIsHovered(true); + }, []); + const handlePointerLeave = useCallback(() => { + setIsHovered(false); + }, []);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/desktop-client/src/components/budget/BudgetTable.jsx (2 hunks)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (4 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/budget/BudgetTable.jsx
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-09-24T20:27:51.684Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
Additional comments not posted (5)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (4)
15-15: LGTM: New imports for progress display featureThe new imports for
useLocalPref,BarGraphVertical, andbarGraphBudgetCategorytype are appropriate for implementing the new progress display feature and managing local preferences.Also applies to: 23-26
247-257: LGTM: Budget calculationsThe new calculations for budget-related values are correct and provide a comprehensive view of the category's financial state.
298-299: LGTM: Conditional rendering of BarGraphVerticalThe conditional rendering of the BarGraphVertical component based on the
showProgresspreference and hover state is implemented correctly.Also applies to: 302-302, 516-527
413-421:⚠️ Potential issueRemove inline hover styles
The use of inline hover styles is not supported in React. Consider moving these styles to a CSS stylesheet or using a CSS-in-JS solution that supports pseudo-classes.
Remove the inline hover styles and create a separate CSS class or styled component:
valueStyle={{ cursor: 'default', margin: 1, padding: '0 4px', borderRadius: 4, - ':hover': { - boxShadow: 'inset 0 0 0 1px ' + theme.mobileAccountShadow, - backgroundColor: theme.tableBackground, - }, }}Then, add these styles to a separate CSS file or use a CSS-in-JS solution that supports pseudo-classes.
Likely invalid or redundant comment.
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1)
Line range hint
219-223: Good implementation of user preference handlingThe use of
useLocalPrefto retrieve and manage theshowProgresspreference is appropriate and correctly implemented. This allows for consistent user experience based on saved preferences.
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (3)
16-44: LGTM: New Legend component enhances UI clarityThe
Legendcomponent is well-implemented and enhances the user interface by providing clear visual cues for the progress bars. It follows React best practices and uses appropriate styling for accessibility.Consider making the component more flexible by allowing customization of the dot size and spacing. You could add optional props like
dotSizeandspacingwith default values:function Legend({ title, color, dotSize = 14, spacing = 5 }: { title: string; color: string; dotSize?: number; spacing?: number; }) { // ... use dotSize and spacing in the style objects }This change would make the component more reusable in different contexts.
58-61: LGTM: BudgetTotals component updated with new visibility controlsThe changes to the
BudgetTotalscomponent successfully implement the new visibility control features. The use ofuseSyncedPreffor getting the budget type and the updates to the menu selection handling are appropriate and align with the PR objectives.To improve code clarity, consider extracting the menu selection handling logic into a separate function. This would make the component more readable and easier to maintain. For example:
const handleMenuSelect = (type: string) => { switch (type) { case 'showHiddenCategories': setShowHiddenCategories(!showHiddenCategories); break; case 'showProgress': setShowProgress(!showProgress); break; case 'expandAllCategories': expandAllCategories(); setMenuOpen(false); break; case 'collapseAllCategories': collapseAllCategories(); setMenuOpen(false); break; } };Then, you can use this function in the
onMenuSelectprop:onMenuSelect={handleMenuSelect}This refactoring would make the component's logic easier to understand and maintain.
Also applies to: 65-65, 119-119, 123-126, 137-145
145-172: LGTM: Legend components enhance menu tooltipThe addition of Legend components in the menu tooltip significantly improves the user's understanding of the progress bar colors. The conditional rendering based on the budget type is implemented correctly and aligns well with the PR objectives.
To improve code organization and reusability, consider extracting the Legend components into a separate function or component. This would make the code more modular and easier to maintain. For example:
const RenderLegends = ({ budgetType }: { budgetType: string }) => ( <View style={{ padding: 10, paddingBottom: 0 }}> {budgetType === 'rollover' && ( <> <Legend title="Rollover added" color={theme.reportsLightPurple} /> <Legend title="Rollover spent" color={theme.reportsLightGreen} /> <Legend title="Rollover overspent" color={theme.reportsLightRed} /> </> )} <Legend title="Budgeted" color={theme.reportsPurple} /> <Legend title="Spent" color={theme.reportsGreen} /> <Legend title="Overspent" color={theme.reportsRed} /> </View> );Then, you can use this component in the
customTooltipprop:customTooltip: <RenderLegends budgetType={budgetType} />This refactoring would improve code organization and make it easier to update or extend the legends in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (2 hunks)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
Additional comments not posted (2)
packages/desktop-client/src/components/budget/BudgetTotals.tsx (2)
4-4: LGTM: New import for useSyncedPrefThe addition of the
useSyncedPrefimport is appropriate for managing synchronized preferences, which aligns with the new visibility management functionality.
48-51: LGTM: New props for enhanced visibility controlThe addition of
setShowHiddenCategories,showHiddenCategories,setShowProgress, andshowProgressprops aligns well with the PR objectives. These props provide granular control over the visibility of hidden categories and progress bars, enhancing the component's flexibility and user control.The prop names are correct and consistent, addressing the typo mentioned in previous reviews.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (1)
Line range hint
314-549: LGTM: Updated JSX for new functionalityThe JSX has been successfully updated to include the new progress bar feature and hover functionality. The conditional rendering of the BarGraphVertical component based on the hover state is implemented correctly.
Enhance accessibility for interactive elements
While the new functionality is well-implemented, consider improving accessibility for screen reader users.
Add appropriate ARIA attributes to the interactive elements. For example:
<Button ref={budgetMenuTriggerRef} variant="bare" onPress={() => setBudgetMenuOpen(true)} + aria-label="Open budget menu" style={{ padding: 3, }} > {/* ... */} </Button>Also, consider adding appropriate aria-labels or aria-describedby attributes to the progress bar elements to provide context for screen reader users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/style/themes/dark.ts (1 hunks)
- packages/desktop-client/src/style/themes/development.ts (1 hunks)
- packages/desktop-client/src/style/themes/light.ts (1 hunks)
- packages/desktop-client/src/style/themes/midnight.ts (1 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
- packages/desktop-client/src/style/themes/dark.ts
- packages/desktop-client/src/style/themes/development.ts
- packages/desktop-client/src/style/themes/light.ts
- packages/desktop-client/src/style/themes/midnight.ts
- packages/loot-core/src/types/prefs.d.ts
🧰 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-09-24T20:27:51.684Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
GitHub Check: typecheck
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
[failure] 10-10:
Duplicate identifier 'css'.
GitHub Check: lint
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
[warning] 10-10:
There should be no empty line within import group
[warning] 10-10:
'/home/runner/work/actual/actual/node_modules/glamor/index.d.ts' imported multiple times
[warning] 262-262:
Delete··
[warning] 266-266:
Delete··
[warning] 269-269:
Insert··
[warning] 270-270:
Insert··
[warning] 271-271:
Insert··
[warning] 272-272:
Insert··
🔇 Additional comments not posted (3)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (3)
19-30: LGTM: New imports for progress bar featureThe new imports for
useLocalPref,BarGraphVertical, andbarGraphBudgetCategoryare correctly added and necessary for the new progress bar feature.
300-312: LGTM: Data structure for bar graphThe data structure for the bar graph is well-defined and includes all necessary information for rendering the progress bar.
Line range hint
1-649: Summary of changes and suggestionsThe changes in this file introduce a new progress bar feature to the budget management system, enhancing the visual representation of budget data. The implementation is generally well-done, with appropriate state management and conditional rendering.
Key points and suggestions:
- Remove the duplicate import of
css.- Consider memoizing the
showProgressvalue for optimization.- Optimize hover state management by simplifying the
handlePointerEnterfunction and adding dependencies to theuseEffecthook.- Refactor complex calculations into separate functions for better maintainability.
- Enhance accessibility by adding appropriate ARIA attributes to interactive elements.
These improvements will help optimize performance, increase maintainability, and improve accessibility of the new feature.
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (1)
Line range hint
312-545: Enhanced UI with improved interactivity and visual representationThe UI changes in the
ExpenseCategoryMonthcomponent significantly improve the user experience:
- Added hover functionality for better interactivity.
- Implemented a conditional rendering of the
BarGraphVerticalcomponent for visual budget representation.- Updated budget menu and balance menu functionality for easier budget management.
These changes provide users with more intuitive ways to interact with and understand their budget data.
However, there's room for improvement in terms of accessibility:
Enhance accessibility for the budget menu button by adding an aria-label:
<Button ref={budgetMenuTriggerRef} variant="bare" onPress={() => setBudgetMenuOpen(true)} + aria-label="Open budget menu" style={{ padding: 3, }} > <SvgCheveronDown width={14} height={14} className="hover-visible" style={budgetMenuOpen ? { opacity: 1 } : {}} /> </Button>This will make it easier for users relying on screen readers to understand the purpose of the button.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/components/reports/graphs/BarGraphVertical.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
- packages/desktop-client/src/components/reports/graphs/BarGraphVertical.tsx
🧰 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-09-24T20:27:51.684Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🔇 Additional comments not posted (3)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (3)
17-17: New imports added for enhanced functionalityThe following new imports have been added:
useLocalPrefhook from '../../../hooks/useLocalPref'barGraphBudgetCategorytype andBarGraphVerticalcomponent from '../../reports/graphs/BarGraphVertical'These imports suggest the addition of local preference management and a new graphical representation of budget data.
Also applies to: 25-28
298-310: Well-structured data for bar graph representationThe
dataarray of typebarGraphBudgetCategory[]is well-organized and includes all necessary information for the bar graph representation. This structure will make it easier to render the graph and manage the budget data visually.
Line range hint
1-645: Summary: Significant improvements to budget management functionalityThis update to the
EnvelopeBudgetComponents.tsxfile, particularly theExpenseCategoryMonthcomponent, brings substantial enhancements to the budget management feature:
- Improved interactivity with hover states and dynamic visual representations.
- Added a bar graph for clearer budget visualization.
- Enhanced menu functionality for easier budget adjustments.
While the implementation is generally well-done, consider the following minor improvements:
- Refactor complex budget calculations into separate functions for better readability.
- Enhance accessibility of the budget menu button.
- Optimize the hover state management by removing unnecessary timeouts.
These changes significantly improve the user experience while maintaining code quality. Great work overall!
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (1)
299-300: Implement hover functionality and progress barThe changes successfully implement the new hover functionality and progress bar feature. The conditional rendering of the
BarGraphVerticalcomponent when hovered aligns well with the PR objectives.Consider adding an
aria-labelto the mainViewcomponent to improve accessibility:<View onMouseEnter={handlePointerEnter} onMouseLeave={handlePointerLeave} + aria-label="Budget category details" style={{ // ...existing styles }} > {/* ... */} </View>Also applies to: 303-303, 519-530
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (1)
216-216: RenameshowProgresstoshowProgressBarfor clarity.The name
showProgressis a bit ambiguous. Renaming it toshowProgressBarwould make it clearer that it specifically controls the visibility of the progress bar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (5 hunks)
- packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (5 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-09-24T20:27:51.684Z Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
[failure] 510-510:
Type '{ disabled: boolean; carryover: "carryover"; balance: "leftover"; goal: "goal"; budgeted: "budget"; longGoal: "long-goal"; }' is not assignable to type 'IntrinsicAttributes & Omit<CellValueProps<SheetNames, SheetFields>, "children" | "binding"> & { ...; }'.
🔇 Additional comments (10)
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (3)
17-17: New feature: Budget progress bar graphThe addition of
useLocalPrefandBarGraphVerticalimports, along with theshowProgresspreference, indicates the implementation of a new budget progress bar graph feature. This aligns well with the PR objectives.Also applies to: 25-28, 206-206
283-295: Well-structured data for budget bar graphThe
datastructure is well-organized and contains all the necessary information for the budget bar graph. This implementation aligns perfectly with the PR objectives.
364-398: Comprehensive BudgetMenu implementationThe BudgetMenu component provides useful functionality for managing budgets, including options to copy last month's average, set months average, and apply budget templates. This implementation enhances user experience and aligns well with the PR objectives.
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx (7)
241-244: ****
246-307: Simplify complex nested ternary expressions for better readability.The nested ternary expressions in the
dataobject can be difficult to read and maintain. Refactoring these expressions into descriptive helper functions or variables will enhance code clarity and maintainability.
334-460: LGTM!The budget cell rendering logic looks good. The editing state is properly handled, and the menu actions are wired up correctly.
461-493: Ensure accessibility by addingaria-labelto interactive elements.The
<span>element used as a button does not have anaria-label, which can hinder accessibility for screen readers. Consider adding anaria-labelor using a button element to improve accessibility.
495-535: LGTM!The balance cell rendering logic looks good. The
BalanceWithCarryovercomponent is used correctly, and the menu actions are wired up properly.🧰 Tools
🪛 GitHub Check: typecheck
[failure] 510-510:
Type '{ disabled: boolean; carryover: "carryover"; balance: "leftover"; goal: "goal"; budgeted: "budget"; longGoal: "long-goal"; }' is not assignable to type 'IntrinsicAttributes & Omit<CellValueProps<SheetNames, SheetFields>, "children" | "binding"> & { ...; }'.
538-548: Optimize rendering performance of the bar graph.Rendering
BarGraphVerticalconditionally on hover may lead to performance issues if the component is complex or if there are many instances ofCategoryMonth. Consider memoizing theBarGraphVerticalcomponent or rendering it unconditionally but controlling its visibility with CSS to improve performance.
509-515:⚠️ Potential issueFix the type error in
BalanceWithCarryoverprops.The static analysis tool reported a type error in the
BalanceWithCarryovercomponent's props. The prop types seem to be incorrectly defined.To verify the type error, run the following script:
If the script confirms the type mismatch, update the
BalanceWithCarryovercomponent's prop types to match the usage:type BalanceWithCarryoverProps = { disabled: boolean; carryover: Binding<'tracking-budget', 'carryover'>; balance: Binding<'tracking-budget', 'leftover'>; goal: Binding<'tracking-budget', 'goal'>; budgeted: Binding<'tracking-budget', 'budget'>; longGoal: Binding<'tracking-budget', 'long-goal'>; };🧰 Tools
🪛 GitHub Check: typecheck
[failure] 510-510:
Type '{ disabled: boolean; carryover: "carryover"; balance: "leftover"; goal: "goal"; budgeted: "budget"; longGoal: "long-goal"; }' is not assignable to type 'IntrinsicAttributes & Omit<CellValueProps<SheetNames, SheetFields>, "children" | "binding"> & { ...; }'.
| const catSumAmount = useEnvelopeSheetValue( | ||
| envelopeBudget.catSumAmount(category.id), | ||
| ); | ||
| const catBudgeted = useEnvelopeSheetValue( | ||
| envelopeBudget.catBudgeted(category.id), | ||
| ); | ||
| const catBalance = useEnvelopeSheetValue( | ||
| envelopeBudget.catBalance(category.id), | ||
| ); | ||
| const carryover = catBalance - catSumAmount - catBudgeted; | ||
| const overSpent = Math.abs(catSumAmount) > catBudgeted + carryover; | ||
|
|
||
| const isCarryoverNegative = carryover < 0; | ||
| const absCatSumAmount = Math.abs(catSumAmount); | ||
| const totalBudget = catBudgeted + carryover; | ||
|
|
||
| // Calculate carryoverSpent | ||
| const carryoverSpent = isCarryoverNegative | ||
| ? null | ||
| : absCatSumAmount < carryover | ||
| ? absCatSumAmount | ||
| : carryover; | ||
|
|
||
| // Calculate carryoverRemaining | ||
| const carryoverRemaining = | ||
| absCatSumAmount < carryover ? carryover + catSumAmount : null; | ||
|
|
||
| // Calculate spent | ||
| const spent = isCarryoverNegative | ||
| ? absCatSumAmount > catBudgeted | ||
| ? null | ||
| : absCatSumAmount < totalBudget | ||
| ? absCatSumAmount | ||
| : totalBudget | ||
| : absCatSumAmount < carryover | ||
| ? null | ||
| : absCatSumAmount < totalBudget | ||
| ? absCatSumAmount - carryover | ||
| : catBudgeted; | ||
|
|
||
| // Calculate remaining | ||
| const remaining = isCarryoverNegative | ||
| ? absCatSumAmount > catBudgeted | ||
| ? null | ||
| : !overSpent && catBudgeted + catSumAmount + carryover | ||
| : absCatSumAmount < carryover | ||
| ? catBudgeted | ||
| : !overSpent && catBudgeted + catSumAmount + carryover; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify complex budget calculations
The budget calculations are correct but use complex nested ternary operators, making the code hard to read and maintain.
Consider refactoring these calculations into separate functions for better readability and maintainability. For example:
function calculateCarryoverSpent(carryover: number, absCatSumAmount: number): number | null {
if (carryover < 0) {
return null;
}
return absCatSumAmount < carryover ? absCatSumAmount : carryover;
}
// Usage in the component
const carryoverSpent = calculateCarryoverSpent(carryover, absCatSumAmount);Apply similar refactoring to other complex calculations in this section.
| <EnvelopeSheetCell | ||
| name="budget" | ||
| exposed={editing} | ||
| focused={editing} | ||
| width="flex" | ||
| onExpose={() => onEdit(category.id, month)} | ||
| style={{ | ||
| borderBottomWidth: showProgress ? 0 : 1, | ||
| ...(editing && { zIndex: 100 }), | ||
| ...styles.tnum, | ||
| }} | ||
| textAlign="right" | ||
| valueStyle={{ | ||
| cursor: 'default', | ||
| margin: 1, | ||
| padding: '0 4px', | ||
| borderRadius: 4, | ||
| ':hover': { | ||
| boxShadow: 'inset 0 0 0 1px ' + theme.mobileAccountShadow, | ||
| backgroundColor: theme.tableBackground, | ||
| }, | ||
| }} | ||
| valueProps={{ | ||
| binding: envelopeBudget.catBudgeted(category.id), | ||
| type: 'financial', | ||
| getValueStyle: makeAmountGrey, | ||
| formatExpr: expr => { | ||
| return integerToCurrency(expr); | ||
| }, | ||
| unformatExpr: expr => { | ||
| return amountToInteger(evalArithmetic(expr, 0)); | ||
| }, | ||
| }} | ||
| inputProps={{ | ||
| onBlur: () => { | ||
| onEdit(null); | ||
| }, | ||
| style: { | ||
| backgroundColor: theme.tableBackground, | ||
| }, | ||
| }} | ||
| onSave={amount => { | ||
| onBudgetAction(month, 'budget-amount', { | ||
| category: category.id, | ||
| amount, | ||
| }); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Improve hover effects in EnvelopeSheetCell
The EnvelopeSheetCell component has been updated with new styling and improved functionality for financial values. However, the use of inline styles for hover effects may not work as expected in React.
Consider moving the hover styles to a CSS stylesheet or using a CSS-in-JS library that supports pseudo-classes. For example, using styled-components:
import styled from 'styled-components';
const StyledCellValueText = styled(CellValueText)`
cursor: default;
margin: 1px;
padding: 0 4px;
border-radius: 4px;
&:hover {
box-shadow: inset 0 0 0 1px ${props => props.theme.mobileAccountShadow};
background-color: ${props => props.theme.tableBackground};
}
`;
// Then use `StyledCellValueText` instead of `CellValueText`This will ensure that the hover effects work as intended.
| const [isHovered, setIsHover] = useState(false); | ||
|
|
||
| const handlePointerEnter = useCallback(() => { | ||
| setIsHover(true); | ||
| }, []); | ||
|
|
||
| const handlePointerLeave = useCallback(() => { | ||
| setIsHover(false); | ||
| }, []); | ||
|
|
||
| // Force closing the tooltip whenever the disablement state changes | ||
| useEffect(() => { | ||
| setIsHover(false); | ||
| }, []); |
There was a problem hiding this comment.
Improve useEffect dependency array
The hover state management is well-implemented. However, the useEffect hook is missing dependencies, which may lead to unexpected behavior.
Consider adding relevant dependencies to the useEffect hook:
useEffect(() => {
setIsHover(false);
- }, []);
+ }, [/* Add relevant dependencies here */]);This will ensure that the effect runs when necessary, improving the reliability of the hover state management.
Committable suggestion was skipped due to low confidence.
| const [isHovered, setIsHover] = useState(false); | ||
|
|
||
| const handlePointerEnter = useCallback(() => { | ||
| setIsHover(true); | ||
| }, []); | ||
|
|
||
| const handlePointerLeave = useCallback(() => { | ||
| setIsHover(false); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting hover state logic into a custom hook.
The hover state logic using useState, useCallback, and useEffect is duplicated in multiple components. Extract it into a reusable custom hook to improve code maintainability and readability.
Here's an example custom hook:
function useHover() {
const [isHovered, setIsHovered] = useState(false);
const handleMouseEnter = useCallback(() => {
setIsHovered(true);
}, []);
const handleMouseLeave = useCallback(() => {
setIsHovered(false);
}, []);
return {
isHovered,
handleMouseEnter,
handleMouseLeave,
};
}Then use it in the component:
const { isHovered, handleMouseEnter, handleMouseLeave } = useHover();
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
Hi I finally had the chance to try this with my budget. First of all I probably misunderstood what this does, as I was expecting a visual way to see how far i am from a budget goal template. Now that I think I understood the various colors to me this feels very complex (also brilliant honestly). To me it is simpler just looking at numbers if I want to have that much insight. Personally (and this is just my budget philosophy, maybe others have different view), I do not find useful knowing that my balance is made up of rollover from the previous month or amount budgeted this month. Same for the spent, I do not find it is useful knowing that I spent the rollover or the portion I budgeted this month. My suggestion (based on my use case) would be to focus on providing less information:
|
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
I know there's a custom user script out there for doing something similar with goal templates but I thought it would be helpful to have something like this in the code base. It's also really useful to see any rollover spending that may otherwise be difficult to decipher as just numbers.
Happy to adjust as needed. Open to any feedback you all have!