-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🔧 Summary Card: Change the font size implementation to be simpler & fix import dashboard #3871
🔧 Summary Card: Change the font size implementation to be simpler & fix import dashboard #3871
Conversation
✅ 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 No assets were 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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
@lelemm I simplified the font sizing a little bit, can you check and see if I broke anything? |
WalkthroughThe changes made in The previous method of binary search for adjusting the font size has been eliminated, replaced by a new calculation for Additionally, the rendering logic has been simplified by removing the off-screen div previously used for measuring text size, now utilizing a single Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
packages/desktop-client/src/components/reports/SummaryNumber.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:
The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (3)
38-56
: Consider character width variations in font size calculationThe current implementation assumes all characters have equal width when calculating font size based on string length. However, different characters (e.g., "1" vs "0") have different widths in most fonts.
Consider using a more precise approach:
- (width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length, + (width * FONT_SIZE_SCALE_FACTOR) / (displayAmount.toString().length * 0.7), // 0.7 is an average character width ratioAlso, consider adding a minimum font size to ensure readability:
const calculatedFontSize = Math.min( + Math.max( (width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length, - height, + height + ), + 10 // minimum font size );
65-90
: Consider layout shift during font size transitionsThe font size transition might cause layout shifts in the parent container. While the animation provides a smooth user experience, it could affect surrounding elements.
Consider these improvements:
- Add
contain: layout
to prevent layout shifts:style={{ alignItems: 'center', flexGrow: 1, flexShrink: 1, width: '100%', height: '100%', + contain: 'layout', maxWidth: '100%',
- Consider using CSS Grid instead of Flexbox for more stable centering:
style={{ - alignItems: 'center', - justifyContent: 'center', + display: 'grid', + placeItems: 'center',
67-68
: Enhance accessibility with more descriptive labelsWhile the current aria-label is functional, it could be more descriptive for screen reader users.
Consider this improvement:
- aria-label={`${value < 0 ? 'Negative' : 'Positive'} amount: ${displayAmount}${suffix}`} + aria-label={`${value < 0 ? 'Negative' : 'Positive'} balance of ${displayAmount}${suffix}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3871.md
is excluded by!**/*.md
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx
(3 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (1)
15-16
: Verify the scaling constants across different scenarios
The FONT_SIZE_SCALE_FACTOR
has been significantly increased from 0.9 to 1.6. While this might improve readability, we should ensure it works well across:
- Different screen sizes
- Various number lengths (e.g., 1000 vs 1000000)
- Different device pixel ratios
Let's verify the component's behavior with different number lengths:
✅ Verification successful
The previous command failed because the --type test
flag wasn't supported. Let's try a different approach to find both test files and usage patterns of the component to verify the scaling factor's impact across different scenarios.
The initial search revealed the component usage in SummaryCard.tsx. Let's dig deeper to understand the actual values being displayed and the font size handling logic to verify if the scaling factor is appropriate.
We need to understand how the font size is calculated and if there are any design system constants that justify the 1.6 scaling factor.
Font size scaling implementation appears reasonable
The scaling factor of 1.6 is well-justified based on the implementation:
- The font size is dynamically calculated considering both container width and text length
- There's a height-based upper limit to prevent overflow
- The calculation
(width * 1.6) / text.length
ensures the text scales proportionally to available space - The component includes smooth transitions and a callback to notify parent components of size changes
The implementation includes safeguards against excessive scaling:
- Math.min() prevents the font from exceeding container height
- ResizeObserver ensures proper resizing on container changes
- The scaling is inversely proportional to text length, preventing long numbers from getting too large
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files or usage of SummaryNumber to understand test coverage
rg -l "SummaryNumber" --type test
Length of output: 62
Script:
#!/bin/bash
# Search for test files related to SummaryNumber
fd ".*test.*" | rg -l "SummaryNumber"
# Search for usage of SummaryNumber component to understand different scenarios
rg "SummaryNumber" -A 3 -B 3
Length of output: 3416
Script:
#!/bin/bash
# Let's look for the actual values being passed to SummaryNumber to understand the range
rg "total.*?}" -A 2 packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
# Also check for any existing font size related code in the component
rg "fontSize|font-size" packages/desktop-client/src/components/reports/SummaryNumber.tsx
Length of output: 648
Script:
#!/bin/bash
# Let's examine the font size calculation logic
rg -A 10 "calculatedFontSize" packages/desktop-client/src/components/reports/SummaryNumber.tsx
# Also check if there are any existing UI components or design system constants that might inform the scaling factor
fd "theme|constants|styles" | rg -l "font|scale"
Length of output: 703
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (3)
39-56
: Consider optimizing the resize handler performanceThe current debounce timeout of 100ms might be too short for frequent resize events. Also, the calculation runs even if the container dimensions haven't changed.
Consider these optimizations:
const handleResize = debounce(() => { if (!refDiv.current) return; const { clientWidth, clientHeight } = refDiv.current; + + // Skip calculation if dimensions haven't changed + if ( + clientWidth === refDiv.current.dataset.lastWidth && + clientHeight === refDiv.current.dataset.lastHeight + ) { + return; + } + + // Store current dimensions + refDiv.current.dataset.lastWidth = clientWidth; + refDiv.current.dataset.lastHeight = clientHeight; const width = clientWidth; const height = clientHeight - CONTAINER_MARGIN * 2; // ... rest of the calculation - }, 100); + }, 150); // Slightly longer debounce for better performance
65-90
: Consider style improvements for better cross-browser supportThe styling implementation is good, but could be enhanced for better compatibility and flexibility.
Consider these style improvements:
style={{ alignItems: 'center', flexGrow: 1, flexShrink: 1, width: '100%', height: '100%', maxWidth: '100%', fontSize, - lineHeight: 1, + lineHeight: '1.2', // Slightly more readable margin: `${CONTAINER_MARGIN}px 0`, justifyContent: 'center', - transition: animate ? 'font-size 0.3s ease' : '', + transition: animate ? 'font-size 0.3s ease, -webkit-font-size 0.3s ease' : '', color: value < 0 ? chartTheme.colors.red : chartTheme.colors.blue, }}
Line range hint
1-94
: Overall architectural assessmentThe simplified approach to font sizing is a good improvement that makes the code more maintainable. The removal of the binary search method in favor of a direct calculation is cleaner and likely more performant. The accessibility implementation is solid with proper ARIA attributes.
Consider documenting the reasoning behind the specific
FONT_SIZE_SCALE_FACTOR
value in a comment, as this seems to be a crucial magic number that future maintainers might need to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx
(3 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
35-35
: LGTM: Improved initial state management
Good improvement initializing fontSize with initialFontSize instead of 0, preventing unnecessary flicker.
Also applies to: 38-38
15-16
: Verify the increased font scale factor maintains proper text display
The FONT_SIZE_SCALE_FACTOR
has been significantly increased from 0.9 to 1.6 (77% larger). While this simplification is good, we should ensure this doesn't cause any overflow issues with different numerical values and container sizes.
Let's verify the impact:
✅ Verification successful
Based on the search results, I can now make an informed assessment about the font size changes in SummaryNumber.tsx:
Font size scaling appears appropriate and consistent with codebase patterns
The increased scale factor (1.6) is justified because:
- The component uses a dynamic calculation that scales based on container width and text length:
(width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length
- The scaling is constrained by
Math.min()
which prevents overflow - The codebase shows consistent patterns of larger font sizes for numerical displays, with examples ranging from 15-20px for prominent numbers
- The component includes proper container margins (8px) to prevent text from touching boundaries
The implementation maintains proper text display while simplifying the calculation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing font size related issues or discussions
rg -i "font.*size|text.*overflow|ellipsis" --type ts --type tsx
# Look for similar font scaling implementations for consistency
ast-grep --pattern 'const $_ = $NUM' | rg -i "font|scale"
Length of output: 19166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/server/dashboard/app.ts (1)
Line range hint
1-248
: Excellent architectural design for widget extensibility!The current implementation demonstrates good architectural practices:
- Adding new widget types requires minimal changes
- Validation, import/export, and database operations automatically handle new types
- Type safety is maintained through TypeScript definitions
This makes the codebase very maintainable and extensible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/dashboard/app.ts
(1 hunks)
🔇 Additional comments (1)
packages/loot-core/src/server/dashboard/app.ts (1)
84-84
: LGTM! Verify type definitions for the new widget type.
The addition of 'summary-card' to the list of valid widget types is correct and well-integrated into the existing validation logic.
Let's verify that the type definitions are properly updated:
✅ Verification successful
Type definitions for 'summary-card' widget are properly implemented
The type definitions for the 'summary-card' widget are correctly implemented in packages/loot-core/src/types/models/dashboard.d.ts
. The widget type is properly defined as SummaryWidget
with all necessary properties including meta fields for conditions, timeFrame, and content. The implementation in Overview.tsx and Summary.tsx also correctly uses these type definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new widget type is properly defined in type definitions
# Search for type definitions related to widget types
rg -t typescript "type.*Widget.*=.*{"
# Search for any references to summary-card that might need updates
rg "summary-card"
Length of output: 616
Script:
#!/bin/bash
# Let's check the type definitions and usage more thoroughly
# Check the type definition file content
cat packages/loot-core/src/types/models/dashboard.d.ts
# Check the implementation in Overview.tsx
cat packages/desktop-client/src/components/reports/Overview.tsx
# Check Summary.tsx implementation
cat packages/desktop-client/src/components/reports/reports/Summary.tsx
Length of output: 36569
LGTM @MikesGlitch |
It just felt right ya know? 😏 I just fixed a small issue where the suffix wasn't being accounted for in the text length - I think that's everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
68-68
: Enhance accessibility with more descriptive aria-labelThe current aria-label only indicates positive/negative amount. Consider including the context (e.g., "Total balance" or "Account summary").
-aria-label={`${value < 0 ? 'Negative' : 'Positive'} amount: ${displayAmount}`} +aria-label={`Summary ${value < 0 ? 'negative' : 'positive'} amount: ${displayAmount}`}
78-80
: Consider performance impact of margin and transitionThe margin and transition properties might cause layout shifts during resizing:
- The margin creates extra space that could cause container jumps
- The font-size transition might cause Cumulative Layout Shift (CLS)
Consider:
- Using padding instead of margin to maintain container stability
- Adding
contain: strict
to prevent layout shifts during transitions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/reports/SummaryNumber.tsx
(3 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
46-49
: 🛠️ Refactor suggestion
Add safeguards and improve font size calculation
The current implementation has several potential issues:
- No minimum font size protection
- No handling of edge cases (empty string, very long text)
- Using string length for proportional fonts might not be accurate
Consider this improved implementation:
const calculatedFontSize = Math.min(
- (width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length,
- height, // Ensure the text fits vertically by using the height as the limiting factor
+ Math.max(
+ (width * FONT_SIZE_SCALE_FACTOR) / Math.max(displayAmount.toString().length, 1),
+ 12 // minimum readable font size
+ ),
+ height,
);
Likely invalid or redundant comment.
15-16
: Verify the impact of the increased scale factor
The FONT_SIZE_SCALE_FACTOR
has been significantly increased from 0.9 to 1.6 (77% increase). While this might help fill the available space better, we should ensure it doesn't cause text overflow in edge cases.
Let's verify the impact:
✅ Verification successful
Font size scaling implementation appears safe and well-controlled
The increased scale factor (1.6) is actually part of a well-implemented font size calculation that:
- Scales based on container width and text length:
(width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length
- Has a height-based upper limit:
Math.min(..., height)
- Includes container margins for vertical space
- Persists calculated sizes through
fontSizeChanged
callback - Updates responsively using ResizeObserver
The implementation ensures text won't overflow regardless of the scale factor value, as it's properly constrained by both width and height of the container.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files or snapshots that might contain assertions about text sizing
rg -l "SummaryNumber" --glob "**/*.test.{ts,tsx,js,jsx}"
rg -l "SummaryNumber" --glob "**/__snapshots__/*.snap"
Length of output: 111
Script:
#!/bin/bash
# Let's check for usage of SummaryNumber component and its font size related props
rg -A 5 "<SummaryNumber"
# Also check the implementation to understand font size calculation logic
ast-grep --pattern 'const calculateFontSize = ($$$) => {
$$$
}'
Length of output: 773
Script:
#!/bin/bash
# Let's examine the SummaryCard component to understand how initialFontSize is used
rg -B 5 -A 10 "initialFontSize" packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
# Also check the current implementation of font size calculation in SummaryNumber
rg -B 5 -A 15 "FONT_SIZE_SCALE_FACTOR" packages/desktop-client/src/components/reports/SummaryNumber.tsx
Length of output: 1973
🤣👌
Reviewers must be maintainers or can I approve this for you? |
Anyone can review, but I'll still need a maintainer to approve before merging. Thanks for checking - I wanted to be sure I wasn't breaking anything 😅 . |
looks good. I wasn't satisfied with the offscreen div neither |
e7aa892
to
a2fbceb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (2)
46-49
: Consider adding font size boundaries for edge casesWhile the simplified calculation is a good improvement, consider adding safeguards:
- Add a minimum font size to ensure text remains readable with very long values
- Consider caching the string length calculation to optimize performance
const calculatedFontSize = Math.min( - (width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length, + Math.max( + (width * FONT_SIZE_SCALE_FACTOR) / displayAmount.toString().length, + 12 // minimum readable font size + ), height, );
39-56
: Consider cleanup and optimization of resize handlerThe debounced resize handler should be cleaned up on component unmount to prevent memory leaks. Also, consider memoizing the callback to prevent unnecessary re-renders.
+ const handleResize = React.useCallback( debounce(() => { if (!refDiv.current) return; // ... existing code ... }, 100), + [displayAmount, initialFontSize, fontSizeChanged] + ); + React.useEffect(() => { + return () => { + handleResize.clear(); + }; + }, [handleResize]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3871.md
is excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/reports/SummaryNumber.tsx
(3 hunks)packages/loot-core/src/server/dashboard/app.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/server/dashboard/app.ts
🔇 Additional comments (3)
packages/desktop-client/src/components/reports/SummaryNumber.tsx (3)
35-37
: Good improvement in state initialization
Using initialFontSize
instead of 0 for the initial state is a good change that prevents unnecessary re-renders and provides better visual stability.
65-87
: Excellent improvements to the rendering logic
The changes show several good improvements:
- Simplified DOM structure by removing the off-screen measurement div
- Better accessibility with proper ARIA attributes
- Clean margin handling with
CONTAINER_MARGIN
- Smooth transitions when
animate
is enabled
15-16
: Verify the scale factor across different viewport sizes
The significant increase in FONT_SIZE_SCALE_FACTOR
from 0.9 to 1.6 (77% increase) could affect text rendering across all summary cards. While this might improve readability, we should ensure it works well across different viewport sizes and text lengths.
✅ Verification successful
Font size scaling implementation is safe and well-constrained
The increased FONT_SIZE_SCALE_FACTOR
(1.6) is safely implemented with proper constraints:
- Font size calculation considers both container width and height as limiting factors
- The component is used within a flex container with proper overflow handling
- The SummaryCard component provides controlled dimensions and proper layout structure
- The implementation includes margin safeguards (
CONTAINER_MARGIN
) to prevent text overflow - Font size changes are persisted through the
fontSizeChanged
callback, allowing for user customization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by this scale factor
rg -l "SummaryNumber" --type tsx
Length of output: 60
Script:
#!/bin/bash
# Search for SummaryNumber usage with proper file extension
rg -l "SummaryNumber" --type ts --type tsx --type jsx --type js
# Also search for any files that might use similar font scaling logic
rg "FONT_SIZE_SCALE" --type ts --type tsx --type jsx --type js
Length of output: 182
Script:
#!/bin/bash
# Search for SummaryNumber usage without file type restrictions
rg -l "SummaryNumber"
# Search for font scale related constants
rg "FONT_SIZE_SCALE"
Length of output: 418
Script:
#!/bin/bash
# Check how SummaryNumber is used in SummaryCard
cat packages/desktop-client/src/components/reports/reports/SummaryCard.tsx
# Look for any width or container size related props or styles
rg -A 5 "width|container" packages/desktop-client/src/components/reports/SummaryNumber.tsx
Length of output: 4768
I've simplified the font sizing - it should have quicker performance and it's easier to understand. There's a slight spacing adjustment too - It should look less tight but still fill out the space.
Will need a thorough check to ensure It has the same output depending on the value.
This also fixes the Import dashboard when a summary card is being imported. summary-card has been added into the importable report types.