SpendingReport: Fixing some display issues #3451
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
|
carkom
changed the title
WalkthroughThe changes involve updates to the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
180-180: Remove unnecessary whitespaceThere's extra whitespace on line 180 that should be removed to adhere to code formatting standards.
Suggested fix:
180-🧰 Tools
GitHub Check: lint
[warning] 180-180:
Delete··
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx (1 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.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)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/desktop-client/src/components/reports/graphs/SpendingGraph.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
🧰 Additional context used
GitHub Check: lint
packages/desktop-client/src/components/reports/reports/Spending.tsx
[warning] 180-180:
Delete··
GitHub Check: typecheck
packages/desktop-client/src/components/reports/reports/Spending.tsx
[failure] 489-489:
Cannot find name 'mode'.
[failure] 491-491:
Cannot find name 'mode'.
[failure] 491-491:
Cannot find name 'mode'.
[failure] 492-492:
Cannot find name 'mode'.
🔇 Additional comments not posted (1)
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
172-177: Code logic for displaying comparison data looks correctThe conditions for
showCompareToandshowCompareare correctly implemented to determine whether comparison data should be displayed based on the current month and data availability.
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/reports/LegendItem.tsx (4)
6-6: LGTM: Component declaration is correct. Consider extracting prop types.The component declaration follows React functional component patterns and uses TypeScript for type safety. For improved readability, especially if the component grows more complex, consider extracting the prop types to a separate interface.
You could refactor it like this:
interface LegendItemProps { color: string; label: string; } export function LegendItem({ color, label }: LegendItemProps) { // ... }
8-14: LGTM: Main container layout is correct. Consider extracting styles.The main View container correctly sets up a row layout with centered items. However, to improve maintainability, consider extracting the inline styles to a separate styles object or using a styling library if one is already in use in the project.
You could refactor it like this:
const styles = { container: { paddingBottom: 10, flexDirection: 'row' as const, alignItems: 'center' as const, }, // ... other styles }; export function LegendItem({ color, label }: LegendItemProps) { return ( <View style={styles.container}> {/* ... */} </View> ); }
15-23: LGTM: Colored circle implementation is correct. Consider adding accessibility attributes.The colored circle View is correctly implemented using appropriate styles. The large borderRadius ensures a circular shape, and the dynamic backgroundColor allows for flexible use.
To improve accessibility, consider adding appropriate ARIA attributes or a role to the View. For example:
<View style={styles.colorIndicator} role="img" aria-label={`Color indicator for ${label}`} > {/* ... */} </View>This change would help screen readers understand the purpose of this visual element.
24-32: LGTM: Label text styling is appropriate. Consider adding text processing.The Text component is styled correctly to prevent wrapping and handle overflow with ellipsis. Setting flexShrink to 0 ensures the text doesn't shrink unnecessarily.
Consider adding some text processing to handle potential edge cases:
<Text style={styles.label}> {label.trim().length > 0 ? label : 'Unnamed'} </Text>This change ensures that empty or whitespace-only labels are replaced with a default value, improving the robustness of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/reports/LegendItem.tsx (1 hunks)
- packages/desktop-client/src/components/reports/ReportLegend.tsx (2 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/reports/reports/Spending.tsx
🔇 Additional comments not posted (5)
packages/desktop-client/src/components/reports/LegendItem.tsx (2)
1-4: LGTM: Imports are correct and follow best practices.The imports are appropriate for the component's needs. Using common components (Text and View) promotes consistency and reusability across the application.
1-35: Overall, the LegendItem component is well-implemented with room for minor enhancements.The component successfully creates a legend item with a color indicator and label. It follows React best practices and uses TypeScript for type safety. Consider the following enhancements for future iterations:
- Extract prop types to a separate interface for better readability.
- Move inline styles to a separate styles object or use a styling library for improved maintainability.
- Add accessibility attributes to the color indicator View.
- Implement basic text processing for the label to handle edge cases.
These suggestions are optimizations that can improve the component's robustness and maintainability without changing its core functionality.
packages/desktop-client/src/components/reports/ReportLegend.tsx (3)
7-7: LGTM: New import statement for LegendItemThe import statement for
LegendItemis correctly formatted and follows React component naming conventions. This change promotes modularity by separating the legend item rendering logic into its own component.
Line range hint
1-54: Overall assessment: Improved code structureThe changes in this file enhance the code structure by introducing the
LegendItemcomponent. This refactoring improves modularity and maintainability without altering the main functionality of theReportLegendcomponent. The changes are well-implemented and align with React best practices.🧰 Tools
Biome
[error] 40-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
43-47: LGTM: Usage of LegendItem componentThe replacement of the inline
Viewcomponent withLegendItemimproves code readability and maintainability. The props are correctly passed, including thekeyprop for list rendering optimization.To ensure the
LegendItemcomponent correctly implements all necessary styling and layout, please run the following script:
|
Retriggered VRT just to make sure it's a flake |
Doesn't look like it, is VRT passing locally? |
Fixed...or so I thought |
Move file settings to the file management area
Fixing display issues in spending report and adding a legend.