Update the budget page to be printable #3780
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. |
jfdoming
force-pushed
the
jfdoming/printable-budget-page
branch
from
e2b93c1 to
1bfd0c5
Compare
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
|
jfdoming
changed the title
WalkthroughThe pull request introduces several changes across multiple components in the desktop client, primarily focusing on enhancing print styles. A new CSS media query for print is added to the 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 (6)
packages/desktop-client/src/hooks/useSpaceMeasure.tsx (2)
9-28: Consider adding z-index and pointer-events to prevent interference.The absolutely positioned measuring elements might interfere with click events on other elements. Consider adding CSS properties to prevent this.
style={{ width: '100%', height: 0, position: 'absolute', + zIndex: -1, + pointerEvents: 'none', }}
5-35: Consider documenting the hook's usage patterns.This hook is well-designed and could be useful beyond just print layouts. Consider adding JSDoc comments to document:
- The hook's purpose and use cases
- Requirements for parent container positioning
- Example usage patterns
packages/desktop-client/src/components/budget/BudgetSummaries.tsx (1)
50-51: Consider preventing layout shifts during printing.While the width calculation is correct, consider caching the measured width when printing starts to prevent potential layout shifts during the print process.
- const totalWidth = isPrinting ? (measure.width() ?? widthState) : widthState; + const measuredWidth = measure.width(); + const [printWidth, setPrintWidth] = useState(0); + + useEffect(() => { + if (isPrinting && measuredWidth) { + setPrintWidth(measuredWidth); + } + }, [isPrinting, measuredWidth]); + + const totalWidth = isPrinting ? (printWidth || measuredWidth || widthState) : widthState;packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (2)
157-181: Consider optimizing the print layout implementation.While the implementation works, there are a few suggestions for improvement:
- The comment "Important: re-render when printing" suggests this is a workaround. Consider adding more detailed documentation explaining why this is necessary.
- The current implementation might cause unnecessary re-renders during printing.
Consider these improvements:
- // Important: re-render when printing to get the correct width + // When printing, we bypass AutoSizer and use fixed dimensions from useSpaceMeasure + // because AutoSizer's measurements aren't reliable in print media context. + // This ensures consistent table dimensions in the printed output. const isPrinting = useMediaQuery('print'); - const measure = useSpaceMeasure(); + // Memoize the measure hook to prevent unnecessary recalculations + const measure = React.useMemo(() => useSpaceMeasure(), []); return ( <> {measure.elements} - <AutoSizer> + <AutoSizer disableHeight={isPrinting} disableWidth={isPrinting}> {({ width, height }) => { if (isPrinting) { return ( <DynamicBudgetTableInner width={measure.width()} height={measure.height()} {...props} /> ); }
Line range hint
1-181: Consider alternative approaches to print layout management.While the current implementation works, it introduces complexity by mixing runtime layout measurement with print media handling. Consider these architectural alternatives:
- Use CSS Grid or Flexbox with print-specific styles to handle layout without JavaScript measurement.
- Create a separate print-optimized view that doesn't require dynamic measurements.
These alternatives could simplify the code and make it more maintainable.
Would you like me to elaborate on these alternative approaches?
packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
Line range hint
19-21: Fix typo in variable namesetShowHiddenCategoriesPef.There appears to be a typo in the variable name. It should be
setShowHiddenCategoriesPrefto maintain consistency with the "preference" terminology.Apply this diff to fix the typo:
const [showHiddenCategories, setShowHiddenCategoriesPef] = useLocalPref( 'budget.showHiddenCategories', ); + const [showHiddenCategories, setShowHiddenCategoriesPref] = useLocalPref( + 'budget.showHiddenCategories', + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3780.mdis excluded by!**/*.md
📒 Files selected for processing (10)
packages/desktop-client/index.html(1 hunks)packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/FinancesApp.tsx(1 hunks)packages/desktop-client/src/components/Titlebar.tsx(1 hunks)packages/desktop-client/src/components/budget/BudgetSummaries.tsx(3 hunks)packages/desktop-client/src/components/budget/BudgetTable.jsx(1 hunks)packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx(4 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/sidebar/Sidebar.tsx(1 hunks)packages/desktop-client/src/hooks/useSpaceMeasure.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/FinancesApp.tsx
- packages/desktop-client/src/components/Titlebar.tsx
- packages/desktop-client/src/components/budget/index.tsx
🔇 Additional comments (11)
packages/desktop-client/src/hooks/useSpaceMeasure.tsx (1)
1-3: LGTM! Clean and minimal imports.
The imports are well-organized with proper separation between external and internal dependencies.
packages/desktop-client/index.html (2)
116-120: LGTM! Print media query implementation looks good.
The addition of the print media query to set body { overflow: visible } is a necessary foundation for making content printable. This change correctly overrides the default overflow: hidden style during printing.
116-120: Verify the impact of global overflow change.
Since this change affects the entire body element, we should ensure it doesn't cause unintended side effects on other pages during printing.
✅ Verification successful
Print overflow styles are consistently applied across components
Based on the search results, the change to make body overflow visible during print is consistent with the application's print styling pattern. Several components already implement the same overflow: visible style for print media:
- FinancesApp component
- App component
- BudgetTable component
- Budget index component
Other print-related styles (in Sidebar and Titlebar) handle visibility appropriately by hiding non-essential UI elements during printing. This change aligns with the existing print styling strategy across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for pages/components that might be affected by the overflow change
# Search for components that explicitly set overflow styles which might conflict with print styles
# Test 1: Find components with overflow styles
echo "Checking for components with overflow styles:"
rg -t tsx -t jsx -t css "overflow:\s*(hidden|auto|scroll)" packages/desktop-client/src
# Test 2: Find existing print-related styles that might interact with this change
echo -e "\nChecking for existing print-related styles:"
rg -t tsx -t jsx -t css "@media\s+print" packages/desktop-client/src
Length of output: 419
Script:
#!/bin/bash
# Let's try without file type restrictions since the previous attempt failed
# and search for overflow styles and print media queries
# Search for overflow styles
echo "Checking for components with overflow styles:"
rg "overflow:\s*(hidden|auto|scroll)" packages/desktop-client
# Search for print-related styles
echo -e "\nChecking for existing print-related styles:"
rg "@media\s+print" packages/desktop-client
Length of output: 1297
Script:
#!/bin/bash
# Let's examine the content of print media queries to understand their interaction with overflow
echo "Checking content of print media queries:"
rg -A 3 "@media\s+print" packages/desktop-client
# Also check the full context of the body overflow in index.html
echo -e "\nChecking context of body overflow in index.html:"
rg -B 5 -A 5 "body\s*{[^}]*overflow:" packages/desktop-client/index.html
Length of output: 2883
packages/desktop-client/src/components/budget/BudgetSummaries.tsx (4)
42-44: LGTM! Clean implementation of print detection.
The print detection is well implemented using the useMediaQuery hook with a clear explanatory comment.
91-95: LGTM! Well-structured animation handling.
The conditional animation logic is well implemented, correctly disabling transforms during printing while maintaining the animation functionality during normal viewing.
78-78: Consider accessibility implications of measurement elements.
While the layout adjustments are correct, ensure that any measurement elements added to the DOM are properly hidden from screen readers and don't interfere with keyboard navigation.
#!/bin/bash
# Description: Check if measurement elements have proper accessibility attributes
# Look for aria-hidden or similar accessibility attributes in the useSpaceMeasure implementation
rg -A 5 "measure\.elements" Also applies to: 86-86
11-11: Verify the implementation of useSpaceMeasure hook.
The imports look good, but let's verify the implementation of the custom useSpaceMeasure hook to ensure it handles print measurements correctly.
Also applies to: 16-16
✅ Verification successful
The useSpaceMeasure hook implementation is correct and appropriate for its usage
The hook is well-implemented and provides the necessary functionality for measuring container dimensions:
- Creates invisible measurement elements for width and height
- Returns width, height functions, and measurement elements
- Properly used in BudgetSummaries for layout calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of useSpaceMeasure hook
# Expected: The hook should handle space measurements, particularly for print media
# Check the implementation
ast-grep --pattern 'export function useSpaceMeasure() {
$$$
}'
# Look for any tests
rg -l "useSpaceMeasure" --glob "**/*.test.*"
Length of output: 1278
Script:
#!/bin/bash
# Let's try a different approach to find the hook implementation
# Search for files containing useSpaceMeasure
rg -l "useSpaceMeasure"
# Search for the actual implementation with context
rg -A 10 "export.*useSpaceMeasure"
# Look for any space measurement related hooks
fd -e ts -e tsx | xargs rg -l "SpaceMeasure"
Length of output: 1351
Script:
#!/bin/bash
# Let's get the complete implementation of useSpaceMeasure hook
cat packages/desktop-client/src/hooks/useSpaceMeasure.tsx
# Also check how it's being used in BudgetSummaries
rg -A 5 "useSpaceMeasure" packages/desktop-client/src/components/budget/BudgetSummaries.tsx
Length of output: 1452
packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (2)
6-10: LGTM! Clean import additions for print functionality.
The new imports for useMediaQuery and useSpaceMeasure are well-organized and align with the PR's objective of making the budget page printable.
Line range hint 120-139: LGTM! Clean implementation of print-specific header handling.
The conditional rendering of BudgetPageHeader based on the print state is well-implemented. This ensures a cleaner print layout by removing the header during printing.
Let's verify that no other components depend on the header being present:
✅ Verification successful
Print layout change for BudgetPageHeader is safe to proceed
The search results confirm that BudgetPageHeader is only used within DynamicBudgetTable.tsx and is properly encapsulated. The component is self-contained and no other components depend on its presence, making it safe to hide during printing. The implementation correctly maintains all necessary props and functionality for normal viewing while cleanly handling the print state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might depend on BudgetPageHeader's presence
rg -l "BudgetPageHeader" | xargs rg "BudgetPageHeader.*props|ref"
Length of output: 65
Script:
#!/bin/bash
# Let's try a different approach to find BudgetPageHeader usage and dependencies
rg -A 5 "BudgetPageHeader" --type typescript --type tsx
Length of output: 90
Script:
#!/bin/bash
# Search for BudgetPageHeader references without file type filters
rg -A 5 "BudgetPageHeader"
Length of output: 2648
packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
216-218: LGTM! Print style changes look good.
The addition of the print media query to make the content fully visible when printing is appropriate and aligns with the PR objective. This ensures that all budget data will be properly rendered in the printed output without being cut off by scroll containers.
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
78-82: LGTM! Verify print styles across components.
The implementation correctly hides the sidebar during printing, which is appropriate for a printable budget page.
Let's verify the consistency of print styles across related components:
✅ Verification successful
Print styles are consistently implemented across components
The verification shows that print-related styles are properly coordinated across components:
- Navigation elements (Sidebar, Titlebar) are hidden with
display: none - Content containers (FinancesApp, Budget components) use
overflow: visibleto ensure proper content rendering - The implementation follows a consistent pattern using emotion's css prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for print-related media queries across components
# Expected: Similar print style implementations in related components
# Search for print-related media queries
rg -l "@media print" "packages/desktop-client/src/components"
# Check specific style implementations
rg -A 3 "@media print.*display.*none" "packages/desktop-client/src/components"
Length of output: 471
Script:
#!/bin/bash
# Let's examine the print-related styles in the identified files
for file in "packages/desktop-client/src/components/FinancesApp.tsx" "packages/desktop-client/src/components/Titlebar.tsx" "packages/desktop-client/src/components/App.tsx" "packages/desktop-client/src/components/sidebar/Sidebar.tsx" "packages/desktop-client/src/components/budget/index.tsx" "packages/desktop-client/src/components/budget/BudgetTable.jsx"; do
echo "=== $file ==="
rg -A 5 "@media print" "$file"
done
Length of output: 4417
|
Using my real budget. yay privacy mode working in the print. Firefox - defaulted to 1 page by condensing it down. Seems overall it thinks the page is much wider than it is? 100% scale is, probably best not done heh. Snippet of pages 2 and 3: Chome has it neatly centered and fine etc. and splits over two pages, Edge is similar, both also show a yellow underline for the demo build notice of the netlify link that Firefox doesn't: Firefox will render background colours correctly: Chrome doesn't render background colours per the theme used, note the dark theme underneath the print window(right side), but light in the print window: This works correctly in firefox: |
|
Any idea on why the print is insisting on starting in October instead of using the current view? It will add more months if I add them, as long as I only want to look at 3 at a time. Any more than 3 and I will still just get 3 in the print preview |
|
Just some thought on the implementation. Would a <PrintAware display="flex" overflow="visible">
<MyComponent />
</PrintAware>PrintAware component would just wrap it's children with a div that has |
|
|
||
| import { View } from '../components/common/View'; | ||
|
|
||
| export const useSpaceMeasure = () => { |
There was a problem hiding this comment.
Would existing useViewportSize/useWindowSize hooks achieve the same thing as this hook?
There was a problem hiding this comment.
This looks cool.
I noticed the budget rows have a larger width than the header - replicated on Edge and firefox:
Also When I select the 5 month display and try to print on Edge it only prints 3 months in the portait. On Firefox it prints 5. It works fine if you print to landscape.
I think these are minor though.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
I've seen this request a few times and got curious what it'd take. Originally I tried the Reports page which seems more difficult, but the Budget page seems doable. Feedback welcome! I've only tested this in Chrome—would appreciate help testing in other browsers.
Suggested testing steps: