✨ polishing report responsitivity #3636
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
|
WalkthroughThis pull request introduces a new React component named Additionally, the 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: 0
🧹 Outside diff range and nitpick comments (16)
packages/desktop-client/src/components/common/SpaceBetween.tsx (2)
7-12: LGTM: Well-defined prop types with a minor enhancement suggestion.The
SpaceBetweenPropstype is clear and well-structured. The use of optional properties provides flexibility, and requiring thechildrenprop ensures the component always has content.Consider adding JSDoc comments to describe each prop, especially for
directionandgap. This would enhance the developer experience when using this component. For example:type SpaceBetweenProps = { /** The direction of the space between items. Defaults to 'horizontal'. */ direction?: 'horizontal' | 'vertical'; /** The size of the gap between items in pixels. Defaults to 15. */ gap?: number; /** Additional CSS styles to apply to the container. */ style?: CSSProperties; /** The content to be rendered within the SpaceBetween component. */ children: ReactNode; };
14-33: LGTM: Well-implemented component with a minor enhancement suggestion.The
SpaceBetweencomponent is concisely implemented and follows React best practices. The use of default prop values and dynamic styles based on thedirectionprop provides excellent flexibility.Consider memoizing the style object to optimize performance, especially if this component is used frequently. You can use the
useMemohook to achieve this:import React, { type ReactNode, useMemo } from 'react'; // ... (rest of the imports and type definition) export const SpaceBetween = ({ direction = 'horizontal', gap = 15, style, children, }: SpaceBetweenProps) => { const containerStyle = useMemo(() => ({ flexWrap: 'wrap' as const, flexDirection: direction === 'horizontal' ? 'row' : 'column', alignItems: 'center' as const, gap, ...style, }), [direction, gap, style]); return ( <View style={containerStyle}> {children} </View> ); };This optimization ensures that the style object is only recalculated when its dependencies change, potentially improving render performance.
packages/desktop-client/src/components/common/Tooltip.tsx (1)
55-55: LGTM! Consider adding a comment for clarity.The addition of
flexShrink: 0is a good improvement. It ensures that the tooltip maintains its size even in constrained spaces, which aligns with the PR's objective of enhancing report responsiveness.Consider adding a brief comment explaining the purpose of
flexShrink: 0. For example:style={{ minHeight: 'auto', flexShrink: 0, // Prevent tooltip from shrinking in constrained spaces }}This will help future developers understand the reasoning behind this style choice.
packages/desktop-client/src/components/filters/FilterExpression.tsx (2)
65-69: Approved with a suggestion for improvementThe new style properties for the Button component effectively manage its layout and prevent overflow issues. However, consider using CSS custom properties (variables) for values like the delete button width to improve maintainability.
Consider refactoring the maxWidth calculation:
- maxWidth: 'calc(100% - 26px)', + maxWidth: `calc(100% - var(--delete-button-width, 26px))`,This change allows for easier updates if the delete button size changes in the future.
71-79: Approved with a suggestion for consistencyThe new style properties for the inner div effectively manage content overflow and improve the visual presentation of the filter expression. However, for consistency and easier maintenance, consider using a single padding shorthand property.
Consider refactoring the padding properties:
- paddingBlock: 1, - paddingLeft: 5, - paddingRight: 2, + padding: '1px 2px 1px 5px',This change maintains the same padding values while making the code more concise and easier to read.
packages/desktop-client/src/components/reports/Header.tsx (3)
79-128: Good use of nested SpaceBetween components.The nested
SpaceBetweencomponents provide excellent control over spacing and layout. The conditional gap in the outer component enhances responsiveness.For consistency, consider using a named constant for the gap value (currently 5) in the inner
SpaceBetween. This would make future adjustments easier and maintain consistency across the component.const INNER_GAP = 5; // ... <SpaceBetween gap={INNER_GAP}> {/* ... */} </SpaceBetween>
Line range hint
131-177: Good use of SpaceBetween for button layout.The
SpaceBetweencomponent effectively manages the spacing between buttons, improving the overall layout.Consider adding
aria-labelattributes to the buttons to enhance accessibility, especially for the "bare" variant buttons. For example:<Button variant="bare" onPress={() => onChangeDates(...getLatestRange(2))} aria-label="View last 3 months" > 3 months </Button>
Line range hint
194-204: Improved layout for AppliedFilters.The addition of a
Viewwrapper forAppliedFilterswith a top margin improves the visual separation and layout control.For consistency with the
SpaceBetweenusage elsewhere in the component, consider replacing theViewwith aSpaceBetween:{filters && filters.length > 0 && ( <SpaceBetween direction="vertical" gap={5}> <AppliedFilters conditions={filters} onUpdate={onUpdateFilter} onDelete={onDeleteFilter} conditionsOp={conditionsOp} onConditionsOpChange={onConditionsOpChange} /> </SpaceBetween> )}This change would maintain consistency in spacing management throughout the component.
packages/desktop-client/src/components/reports/ReportSidebar.tsx (3)
Line range hint
181-200: LGTM: Improved layout with SpaceBetween componentThe introduction of the
SpaceBetweencomponent here improves the layout and spacing of the "Mode" selection buttons. Thegapprop ensures consistent spacing between children, and thestyleprop adds appropriate padding.Consider extracting the inline style object to a separate constant or using a predefined style from your theme to improve readability and maintainability.
Line range hint
395-430: LGTM: Improved layout for Date filters sectionThe use of the
SpaceBetweencomponent here enhances the layout and spacing of the "Date filters" section. Thegapprop ensures consistent spacing between children, and thestyleprop adds appropriate margins.For consistency with the previous
SpaceBetweenusage, consider using padding instead of margin in thestyleprop. This would make the spacing behavior more uniform across the component.
Line range hint
1-563: Overall improvement in layout consistency and responsivenessThe introduction of the
SpaceBetweencomponent in key areas of theReportSidebarhas improved the layout consistency and responsiveness of the UI. These changes align well with the PR objective of "polishing report responsitivity".To further enhance the component's consistency and maintainability, consider:
- Standardizing the use of
SpaceBetweenthroughout the component, replacing otherViewcomponents used for layout where appropriate.- Extracting common style properties (like gap sizes and padding/margin values) into constants or theme variables to ensure consistency and ease future adjustments.
- Reviewing the remaining
Viewcomponents to see if they can benefit from theSpaceBetweencomponent's spacing capabilities.These additional refactors could further improve the overall responsiveness and maintainability of the
ReportSidebarcomponent.packages/desktop-client/src/components/reports/reports/Spending.tsx (5)
239-239: LGTM: SpaceBetween improves layout managementThe introduction of
SpaceBetweenhere improves code readability and maintainability. It provides a more semantic way to manage the layout of child components.Consider setting the
gapprop to a named constant or theme variable (e.g.,theme.spacing.none) instead of a hard-coded 0, to maintain consistency across the application.
261-261: LGTM: SpaceBetween enhances form layoutThe use of
SpaceBetweenhere improves the visual consistency and maintainability of the form layout.Consider setting the
gapprop to a named constant or theme variable (e.g.,theme.spacing.small) instead of a hard-coded 5, to maintain consistency and allow for easier global adjustments.
304-304: LGTM: Separator margins adjustedThe adjustments to the separator's margins ensure proper spacing with the new
SpaceBetweenlayout.Consider using theme variables for margin values (e.g.,
theme.spacing.medium) to maintain consistency across the application and allow for easier global adjustments.
308-308: LGTM: SpaceBetween improves mode button layoutThe use of
SpaceBetweenhere enhances the visual consistency and maintainability of the mode selection button group.Consider setting the
gapprop to a named constant or theme variable (e.g.,theme.spacing.small) instead of a hard-coded 5, to maintain consistency and allow for easier global adjustments. This suggestion applies to all instances ofSpaceBetweenin this file.
Line range hint
26-395: Overall: Improved layout management with SpaceBetweenThe introduction of the
SpaceBetweencomponent throughout this file significantly improves layout management, code readability, and maintainability. The changes are consistent and well-implemented.To further enhance consistency and maintainability, consider the following refactoring:
- Create a set of named constants or use existing theme variables for spacing values (e.g.,
SPACING_NONE,SPACING_SMALL,SPACING_MEDIUM).- Replace all hard-coded values in
gapprops and margin styles with these constants.- Use these constants consistently across all
SpaceBetweencomponents and related styles in this file.This refactoring will make it easier to maintain consistent spacing throughout the application and allow for global adjustments if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3636.mdis excluded by!**/*.md
📒 Files selected for processing (9)
- packages/desktop-client/src/components/common/SpaceBetween.tsx (1 hunks)
- packages/desktop-client/src/components/common/Tooltip.tsx (1 hunks)
- packages/desktop-client/src/components/filters/FilterExpression.tsx (1 hunks)
- packages/desktop-client/src/components/filters/FiltersButton.tsx (1 hunks)
- packages/desktop-client/src/components/reports/Header.tsx (4 hunks)
- packages/desktop-client/src/components/reports/ModeButton.tsx (0 hunks)
- packages/desktop-client/src/components/reports/ReportSidebar.tsx (5 hunks)
- packages/desktop-client/src/components/reports/reports/CustomReport.tsx (0 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.tsx (6 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/reports/ModeButton.tsx
- packages/desktop-client/src/components/reports/reports/CustomReport.tsx
🧰 Additional context used
🔇 Additional comments (7)
packages/desktop-client/src/components/filters/FiltersButton.tsx (1)
9-12: Approved: Improved layout consistency for the filter iconThe addition of
flexShrink: 0to theSvgFiltercomponent's style is a good improvement. This change prevents the icon from shrinking when the button is resized, ensuring consistent layout and appearance across different screen sizes and resolutions. This minor adjustment contributes to the overall goal of enhancing report responsiveness and polishing the UI.packages/desktop-client/src/components/common/SpaceBetween.tsx (1)
1-5: LGTM: Imports are appropriate and well-structured.The imports are correctly defined and include all necessary dependencies for the component. The use of relative paths indicates a well-organized project structure.
packages/desktop-client/src/components/reports/Header.tsx (3)
13-13: LGTM: SpaceBetween import added correctly.The import statement for the new
SpaceBetweencomponent is correctly placed and uses the appropriate path.
73-78: Great improvement in responsiveness!The use of
SpaceBetweenwith conditional direction and alignment based onisNarrowWidthenhances the component's responsiveness. This change effectively replaces the previous static layout with a more flexible one.
Line range hint
1-207: Overall, excellent improvements to layout and responsiveness.The changes in this file significantly enhance the
Headercomponent's layout and responsiveness, aligning well with the PR's objective of "polishing report responsitivity". The introduction ofSpaceBetweencomponents provides better control over spacing and alignment, especially in responsive scenarios.Key improvements:
- Enhanced responsiveness using conditional layouts based on screen width.
- Improved spacing management throughout the component.
- Better organization of nested components.
These changes maintain the existing functionality while making the UI more flexible and maintainable. Great job on the refactoring!
packages/desktop-client/src/components/reports/ReportSidebar.tsx (1)
17-17: LGTM: New import for SpaceBetween componentThe import statement for the
SpaceBetweencomponent has been correctly added. This new component will likely improve the layout and spacing management within theReportSidebar.packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
26-26: LGTM: SpaceBetween import added correctlyThe import for the
SpaceBetweencomponent is correctly placed and follows the existing import structure.
| @@ -52,7 +52,7 @@ export const Tooltip = ({ | |||
|
|
|||
| return ( | |||
| <View | |||
| style={{ minHeight: 'auto' }} | |||
| style={{ minHeight: 'auto', flexShrink: 0 }} | |||
There was a problem hiding this comment.
fix for the tiny icons
| @@ -62,8 +62,21 @@ export function FilterExpression<T extends RuleConditionEntity>({ | |||
| variant="bare" | |||
| isDisabled={customName != null} | |||
| onPress={() => setEditing(true)} | |||
| style={{ | |||
There was a problem hiding this comment.
fix for filters expanding out-of-bounds
| @@ -6,7 +6,10 @@ import { Button } from '../common/Button2'; | |||
| export function FiltersButton({ onPress }: { onPress: () => void }) { | |||
| return ( | |||
| <Button variant="bare" onPress={onPress}> | |||
| <SvgFilter style={{ width: 12, height: 12, marginRight: 5 }} /> Filter | |||
| <SvgFilter | |||
There was a problem hiding this comment.
Fix the the icon in "Filters" button becoming small when resizing browser
| flexShrink: 0, | ||
| }} | ||
| > | ||
| <View | ||
| <SpaceBetween |
There was a problem hiding this comment.
Various small re-alignments in cashflow & networth pages on small devices (i.e. mobile). To make the filters look slightly better on small resolutions.
| @@ -177,16 +178,13 @@ export function ReportSidebar({ | |||
| <strong>Display</strong> | |||
| </Text> | |||
| </View> | |||
| <View | |||
| <SpaceBetween | |||
There was a problem hiding this comment.
No user-facing update here. Just a small code improvement.
| @@ -23,6 +23,7 @@ import { Block } from '../../common/Block'; | |||
| import { Button } from '../../common/Button2'; | |||
| import { Paragraph } from '../../common/Paragraph'; | |||
| import { Select } from '../../common/Select'; | |||
| import { SpaceBetween } from '../../common/SpaceBetween'; | |||
There was a problem hiding this comment.
Improving how the filters look in mobile & smaller resolutions.
MatissJanis
changed the title
|
Found one small thing but it's unrelated to your PR. If you're interested, it's to do with the overflow of the toolbar :
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/filters/CompactFiltersButton.tsx (2)
8-8: LGTM! Consider using a theme variable for consistency.The addition of a minimum width to the Button component is a good improvement for layout stability. This change aligns well with the PR objective of enhancing report responsiveness.
Consider using a theme variable or constant for the
minWidthvalue (20px) to ensure consistency across the application. This would make it easier to maintain and adjust the compact button sizes globally if needed in the future.
9-13: LGTM! Consider optimizing to reduce redundancy.The changes to the SvgFilter component improve layout stability and responsiveness, which aligns well with the PR objectives. Setting explicit dimensions and preventing shrinking are good practices for icon components.
To optimize the code and reduce redundancy, consider consolidating the width and height values:
<SvgFilter - width={15} - height={15} - style={{ width: 15, height: 15, flexShrink: 0 }} + style={{ width: 15, height: 15, flexShrink: 0 }} />This change maintains the same functionality while making the code more concise and easier to maintain.
packages/desktop-client/src/components/reports/ReportTopbar.tsx (2)
74-74: Consider responsive design for overflow handling.The addition of
overflowY: 'auto'improves responsiveness by allowing vertical scrolling when needed. However, consider if this is the best approach for all screen sizes.You might want to implement a more responsive design that adjusts the layout for different screen sizes before resorting to scrolling. For example:
import { useMediaQuery } from 'react-responsive'; // Inside your component const isSmallScreen = useMediaQuery({ maxWidth: 768 }); // In your JSX <View style={{ flexDirection: isSmallScreen ? 'column' : 'row', alignItems: isSmallScreen ? 'flex-start' : 'center', overflowY: isSmallScreen ? 'auto' : 'visible', // ... other styles }} > {/* Your existing content */} </View>This approach would stack the elements vertically on smaller screens and enable scrolling only when necessary.
193-219: Good use of SpaceBetween for improved layout.The introduction of the
SpaceBetweencomponent enhances the layout structure and responsiveness. The custom styling ensures proper spacing and alignment of theFilterButtonandSaveReportcomponents.Consider extracting the inline styles to a separate constant or using a styled-component for better readability:
const spaceBetweenStyles = { flexWrap: 'nowrap' as const, justifyContent: 'space-between' as const, flex: 1, }; // In your JSX <SpaceBetween style={spaceBetweenStyles}> {/* ... */} </SpaceBetween>This approach would make the component more readable and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (24)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
- packages/desktop-client/src/components/filters/CompactFiltersButton.tsx (1 hunks)
- packages/desktop-client/src/components/reports/ReportTopbar.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/desktop-client/src/components/filters/CompactFiltersButton.tsx (1)
Line range hint
1-15: Overall, good improvements to component responsiveness.The changes made to the CompactFiltersButton component contribute positively to the PR's objective of enhancing report responsiveness. The modifications to both the Button and SvgFilter components improve layout stability and consistency.
While the changes are good as they are, consider the minor suggestions provided above to further optimize the code and maintain consistency across the application.
packages/desktop-client/src/components/reports/ReportTopbar.tsx (3)
17-17: LGTM: SpaceBetween import added correctly.The import statement for the
SpaceBetweencomponent is properly placed and follows the existing import structure.
200-218: LGTM: FilterButton and SaveReport properly integrated.The
FilterButtonandSaveReportcomponents are correctly placed within theSpaceBetweencomponent. Their functionality remains intact, and the new layout improves their spacing and alignment.
Line range hint
1-224: Overall, good improvements to ReportTopbar responsiveness.The changes made to this file successfully enhance the responsiveness and layout of the ReportTopbar component. The introduction of the
SpaceBetweencomponent and the adjustments to the styling contribute to a more flexible and maintainable structure. The suggestions provided are minor and aimed at further improving code readability and responsiveness.
|
Good idea @MikesGlitch . Updated the report topbar to scale better. It's not ideal.. but better than before. |
Related #3282