✨ (dashboards) ability to quick-edit widget names from inner report pages #3587
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
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
|
| @@ -73,141 +70,140 @@ export function Header({ | |||
| flexShrink: 0, | |||
| }} | |||
| > | |||
| {!['/reports/custom'].includes(path) && ( | |||
There was a problem hiding this comment.
This component is not used in the custom reports page anymore.
WalkthroughThis pull request introduces a new React component, Additionally, modifications are made to several existing components, including 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: 3
🧹 Outside diff range and nitpick comments (10)
packages/desktop-client/src/components/EditablePageHeaderTitle.tsx (2)
34-55: LGTM: Editing mode render logic is well-implemented, with a minor suggestion.The conditional rendering for the editing mode is correctly implemented with appropriate event handlers. The use of
InitialFocusis a nice touch for improved UX. The dynamic width calculation for the input is clever and ensures a good user experience.Consider moving the inline styles to a separate stylesheet or using a styled-component for better maintainability. This would make the component more readable and easier to maintain in the future.
57-89: LGTM: View mode render logic is well-implemented, with suggestions for improvement.The view mode rendering is well-structured with a good use of layout and hover effects. The edit button implementation with hover visibility is a nice UX touch.
- Consider moving the inline styles to a separate stylesheet or using styled-components for better maintainability and readability.
- To improve accessibility, consider adding an
aria-labelto the edit button to describe its purpose for screen readers. For example:<Button aria-label="Edit title" // ... other props >packages/loot-core/src/server/reports/app.ts (2)
143-143: Improved error message clarity and consistency.The change from "filter" to "report" in the error message accurately reflects the context of the function, which is updating a report. This modification enhances the clarity of the error message and aligns with the PR objectives.
For consistency with the
createReportfunction, consider extracting the error message to a constant. This would make future updates easier and ensure consistency across both functions. For example:const REPORT_NAME_EXISTS_ERROR = (name: string) => `There is already a report named ${name}`; // Then use it in both functions: throw new Error(REPORT_NAME_EXISTS_ERROR(item.name));
Line range hint
1-164: Suggestions for overall improvementsThe file structure and logic are well-implemented. Here are some suggestions for further improvements:
Consider creating a constant for the 'custom_reports' table name to avoid repetition and make future changes easier.
The
reportNameExistsfunction could be simplified by using a single query with a WHERE clause that handles both new and existing reports. This would reduce code duplication and potentially improve performance.Error messages could be centralized into an object or enum for easier management and consistency.
Consider adding more specific error types (e.g.,
ReportNameExistsError) to allow for more granular error handling on the client side.The
reportModel.validatefunction could benefit from stronger typing, possibly using a library like Zod for runtime type checking.Would you like me to provide code examples for any of these suggestions?
packages/desktop-client/src/components/reports/reports/NetWorth.tsx (2)
150-163: LGTM: Well-implemented onSaveWidgetName functionThe
onSaveWidgetNamefunction is well-implemented and correctly handles saving the new widget name. It includes proper error handling and uses a default name if necessary. The server update logic is correct.Consider adding a success notification similar to the one in the
onSaveWidgetfunction to provide feedback to the user when the widget name is successfully updated.
180-191: LGTM: Well-implemented editable title in PageHeaderThe changes to the PageHeader component correctly implement the editable title feature. The conditional rendering ensures that the EditablePageHeaderTitle is only shown when a widget exists, which is the correct behavior.
For consistency with the rest of the codebase, consider using optional chaining (
widget?.) instead of thewidget ? ... : ...ternary operator. This would make the code more concise and align with the coding style used elsewhere in the file.packages/desktop-client/src/components/reports/reports/CashFlow.tsx (3)
171-185: LGTM: Title declaration and onSaveWidgetName function.The title declaration and
onSaveWidgetNamefunction are well-implemented. The function correctly handles edge cases and updates the widget metadata.Consider adding a success notification after saving the widget name, similar to the
onSaveWidgetfunction:dispatch( addNotification({ type: 'message', message: t('Widget name successfully updated.'), }), );
204-215: LGTM: PageHeader title rendering logic.The conditional rendering of the
EditablePageHeaderTitlecomponent is well-implemented. It correctly handles the cases with and without a widget.To improve readability, consider extracting the title rendering logic into a separate variable:
const headerTitle = widget ? ( <EditablePageHeaderTitle title={title} onSave={onSaveWidgetName} /> ) : title; return ( <PageHeader title={headerTitle} /> );
Line range hint
1-365: Overall implementation aligns well with PR objectives.The changes successfully implement the ability to quick-edit widget names from the CashFlow report page, which is consistent with the PR objectives. The feature is well-integrated into the existing component structure without introducing unnecessary complexity.
To further improve the implementation:
- Consider extracting the widget name editing logic into a custom hook (e.g.,
useEditableWidgetName) to promote reusability across different report types.- Implement error handling for the
onSaveWidgetNamefunction to gracefully handle potential API errors.packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
185-199: LGTM: New title logic and onSaveWidgetName functionThe changes improve the widget name handling and add the ability to save updated names. The fallback to a translated string is a good practice.
A minor suggestion:
Consider extracting the default name
t('Monthly Spending')into a constant at the top of the file to avoid repetition:+const DEFAULT_WIDGET_NAME = t('Monthly Spending'); // ... -const title = widget?.meta?.name || t('Monthly Spending'); +const title = widget?.meta?.name || DEFAULT_WIDGET_NAME; // ... -const name = newName || t('Monthly Spending'); +const name = newName || DEFAULT_WIDGET_NAME;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3587.mdis excluded by!**/*.md
📒 Files selected for processing (6)
- packages/desktop-client/src/components/EditablePageHeaderTitle.tsx (1 hunks)
- packages/desktop-client/src/components/reports/Header.tsx (1 hunks)
- packages/desktop-client/src/components/reports/reports/CashFlow.tsx (3 hunks)
- packages/desktop-client/src/components/reports/reports/NetWorth.tsx (3 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.tsx (3 hunks)
- packages/loot-core/src/server/reports/app.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (14)
packages/desktop-client/src/components/EditablePageHeaderTitle.tsx (5)
1-9: LGTM: Imports are well-organized and necessary.The imports are logically structured and include all the necessary components and utilities for the
EditablePageHeaderTitlecomponent.
11-14: LGTM: Props type is well-defined.The
EditablePageHeaderTitlePropstype clearly defines the expected props for the component, with appropriate types for both thetitleandonSaveproperties.
16-23: LGTM: Component definition and state management are well-implemented.The component is defined following React best practices. The state initialization and synchronization with props using
useEffectare correctly implemented. Good job on renaming thetitleprop toinitialTitleto avoid confusion with thetitlestate.
25-32: LGTM: Save callback is well-implemented with proper memoization.The
onSaveValuecallback is correctly memoized usinguseCallback. It properly updates both the parent component and the internal state, and exits edit mode after saving. This implementation ensures good performance and user experience.
1-89: Overall assessment: Well-implemented component with minor suggestions for improvement.The
EditablePageHeaderTitlecomponent is a solid implementation that meets the PR objective of allowing quick editing of widget names. It follows React best practices, uses hooks effectively, and provides a good user experience.Key strengths:
- Clear and concise props definition
- Effective use of React hooks for state management
- Good separation of editing and view modes
- Clever input width calculation for a smooth editing experience
Suggestions for improvement:
- Move inline styles to a separate stylesheet or use styled-components
- Enhance accessibility by adding appropriate ARIA labels
This component should integrate well with the existing codebase and provide the desired functionality for editing widget names from inner report pages.
packages/loot-core/src/server/reports/app.ts (2)
123-123: Improved error message clarity.The change from "filter" to "report" in the error message accurately reflects the context of the function, which is creating a report. This modification enhances the clarity of the error message and aligns with the PR objectives.
Line range hint
1-164: Conclusion: Changes improve clarity and align with PR objectivesThe modifications to the error messages in this file enhance clarity and accurately reflect the context of report operations. These changes align well with the PR objectives of improving dashboard functionality related to reports.
The overall structure and functionality of the file are sound, with appropriate error handling and TypeScript usage. The suggestions provided for further improvements could enhance code maintainability and error handling in the future.
packages/desktop-client/src/components/reports/reports/NetWorth.tsx (3)
23-23: LGTM: Import statement for EditablePageHeaderTitleThe import statement for
EditablePageHeaderTitleis correctly placed and aligns with the new feature for editable widget titles.
149-149: LGTM: Improved title fallback logicThe change from nullish coalescing (
??) to logical OR (||) operator improves the title assignment logic. This ensures that ifwidget?.meta?.nameis falsy (including an empty string), the default title 'Net Worth' will be used, which is the desired behavior.
Line range hint
1-265: Summary: Well-implemented editable widget title featureThe changes in this file successfully implement the ability to edit widget names from inner report pages, which aligns with the PR objectives. The implementation is clean, consistent, and follows good coding practices. The new
EditablePageHeaderTitlecomponent is properly integrated, and the necessary logic for saving the updated widget name is in place.A few minor suggestions have been made for further improvement:
- Adding a success notification when the widget name is updated.
- Using optional chaining for consistency in the PageHeader component.
Overall, these changes effectively enhance the user experience by allowing quick editing of widget names directly from the Net Worth report page.
packages/desktop-client/src/components/reports/reports/CashFlow.tsx (1)
29-29: LGTM: Import statement for EditablePageHeaderTitle.The import statement for
EditablePageHeaderTitleis correctly placed and consistent with its usage in the component.packages/desktop-client/src/components/reports/reports/Spending.tsx (3)
29-29: LGTM: New import for EditablePageHeaderTitleThe import for
EditablePageHeaderTitleis correctly placed and aligns with the PR objective of allowing quick editing of widget names.
212-223: LGTM: Integration of EditablePageHeaderTitleThe PageHeader component has been successfully updated to include the EditablePageHeaderTitle when a widget is present. This change aligns well with the PR objective of allowing quick editing of widget names from inner report pages.
The conditional rendering and prop passing are implemented correctly.
Line range hint
1-624: Summary: Successful implementation of inline widget name editingThe changes in this file successfully implement the ability to quick-edit widget names from inner report pages, as per the PR objectives. The implementation is clean, well-integrated, and follows good practices such as error handling and fallback to default values.
Key improvements:
- Addition of the EditablePageHeaderTitle component
- Implementation of the onSaveWidgetName function
- Integration of the editable title in the PageHeader
These changes enhance the user experience by allowing seamless editing of widget names directly from the report page.
MatissJanis
changed the title
#3282