Adding a help modal for quick reference to goal template syntax #3691
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
|
|
Looks pretty good. Just need to show the option only if templates are enabled. fyi, if you run |
Thanks, lint is fixed, looking into showing it conditionally. |
deathblade666
changed the title
WalkthroughThe pull request introduces several enhancements to the In Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (4)
packages/desktop-client/src/components/HelpMenu.tsx (2)
70-70: LGTM: New feature flag for goal templatesThe
showGoalTemplatesconstant is correctly implemented using theuseFeatureFlaghook. This aligns with the PR objectives of conditionally displaying the goal templates help option.Consider adding a brief comment explaining the purpose of this feature flag for better code readability:
// Feature flag to conditionally render the goal templates help option const showGoalTemplates = useFeatureFlag('goalTemplatesEnabled');
116-118: LGTM: Updated Menu items arrayThe Menu items array has been correctly updated to conditionally include the goal templates menu item. The implementation is clean and efficient, using the spread operator and correctly applying the feature flag and page condition.
For improved readability, consider extracting the condition into a separate constant:
const showGoalTemplatesMenuItem = showGoalTemplates && page === '/budget'; items={[ // ... other items ...(showGoalTemplatesMenuItem ? [{ name: 'goal-templates', text: t('Goal templates') }] : []), ]}This change would make the condition more explicit and easier to understand at a glance.
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx (1)
7-15: LGTM: Component structure is well-organized.The
GoalTemplateModalcomponent is structured effectively, using a functional component and the render prop pattern for the modal. This approach provides good encapsulation and flexibility.Consider extracting the modal content into a separate component to improve readability and maintainability, especially if the content grows larger in the future. For example:
const GoalTemplateContent = () => ( <View style={{ flexDirection: 'row', fontSize: 13 }}> {/* ... existing content ... */} </View> ); export function GoalTemplateModal() { return ( <Modal name="goal-templates"> {({ state: { close } }) => ( <> <ModalHeader title="Goal Templates" rightContent={<ModalCloseButton onPress={close} />} /> <GoalTemplateContent /> </> )} </Modal> ); }Also applies to: 84-88
packages/desktop-client/src/components/Modals.tsx (1)
87-89: LGTM: Implementation of 'goal-templates' caseThe implementation of the 'goal-templates' case is correct and consistent with other modal renderings. The conditional rendering based on
budgetIdis a good practice to ensure the modal is only shown when a budget is open.For improved readability, consider using an early return:
case 'goal-templates': - return budgetId ? <GoalTemplateModal key={name} /> : null; + if (!budgetId) return null; + return <GoalTemplateModal key={name} />;This change would make the code slightly more readable and consistent with the 'keyboard-shortcuts' case implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3691.mdis excluded by!**/*.md
📒 Files selected for processing (4)
- packages/desktop-client/src/components/HelpMenu.tsx (5 hunks)
- packages/desktop-client/src/components/Modals.tsx (2 hunks)
- packages/desktop-client/src/components/modals/GoalTemplateModal.tsx (1 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
packages/desktop-client/src/components/HelpMenu.tsx (4)
11-11: LGTM: New import for useFeatureFlagThe import statement for
useFeatureFlagis correctly placed and aligns with the new feature flag functionality being introduced.
20-20: LGTM: Updated HelpMenuItem typeThe HelpMenuItem type has been correctly updated to include 'goal-templates', ensuring type safety for the new menu item.
86-88: LGTM: Updated handleItemSelect functionThe handleItemSelect function has been correctly updated to include the new 'goal-templates' case. The implementation is consistent with other menu items and correctly dispatches the pushModal action.
Line range hint
1-124: Overall assessment: Well-implemented feature additionThe changes to the HelpMenu component successfully introduce the new goal templates help option. The implementation is consistent with the existing codebase and correctly uses feature flags and page context to conditionally render the new menu item. The code is well-structured and type-safe.
Key points:
- New import and type definition added correctly.
- Feature flag implemented for conditional rendering.
- handleItemSelect function updated to handle the new menu item.
- Menu items array updated with conditional logic for the new item.
The changes align well with the PR objectives and enhance the user experience by providing quick access to goal template syntax help. The minor suggestions provided in the review comments can further improve code readability and maintainability.
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx (2)
1-5: LGTM: Imports are appropriate and concise.The imports are well-organized and all seem to be utilized within the component. Good job on keeping the dependencies minimal and relevant.
1-88: Overall assessment: Well-implemented component with room for enhancement.The
GoalTemplateModalcomponent is a valuable addition to the application, providing users with quick access to goal template syntax examples. The structure is clean, and the content is informative.Key strengths:
- Clear organization of different template types.
- Concise examples that align with the documentation.
- Inclusion of a link to further documentation.
Suggested improvements:
- Enhance accessibility with semantic HTML and ARIA attributes.
- Prepare for internationalization by extracting text content.
- Improve the external link's security and user experience.
- Consider breaking down the component for better maintainability.
These enhancements will further improve the component's quality and usability. Great work on implementing this helpful feature!
packages/loot-core/src/client/state-types/modals.d.ts (2)
289-289: Consider additional updates for the new modal type.While the addition of the 'goal-templates' modal type is straightforward and non-disruptive, consider the following:
- Ensure that any components or functions that handle modal rendering or management (e.g., a modal manager or router) are updated to recognize and handle this new modal type.
- Update relevant documentation, such as API docs or developer guides, to include information about this new modal option.
To help identify areas that might need updating, run the following command:
#!/bin/bash # Find potential areas in the codebase that handle modal types rg "ModalType|FinanceModals" --type ts
289-289: New modal type added for goal templates.The addition of the 'goal-templates' modal type as an EmptyObject is consistent with the PR objectives. It's correctly placed within the FinanceModals type and follows the existing pattern for modals that don't require additional properties.
To ensure this change doesn't introduce any conflicts, let's verify its uniqueness:
✅ Verification successful
'goal-templates' modal type is uniquely and correctly defined.
No conflicts found in the codebase. The 'goal-templates' modal type is properly integrated into relevant components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that 'goal-templates' is not defined elsewhere in the codebase rg "'goal-templates':" --type tsLength of output: 1793
Script:
#!/bin/bash # Verify that 'goal-templates' is not defined elsewhere in the codebase rg "'goal-templates':" --type tsLength of output: 289
packages/desktop-client/src/components/Modals.tsx (2)
38-38: LGTM: Import statement for GoalTemplateModalThe import statement for
GoalTemplateModalis correctly placed and follows the existing pattern for importing modal components.
38-38: Verify the implementation of GoalTemplateModalThe changes to support the new 'goal-templates' modal are well-integrated. To ensure completeness, please verify that the
GoalTemplateModalcomponent exists and is implemented correctly in the./modals/GoalTemplateModalfile.Run the following script to check for the existence and basic structure of the GoalTemplateModal component:
Also applies to: 87-89
✅ Verification successful
GoalTemplateModal Implementation Verified
The
GoalTemplateModalcomponent exists and is implemented correctly inpackages/desktop-client/src/components/modals/GoalTemplateModal.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of GoalTemplateModal # Check if the file exists if [ ! -f "packages/desktop-client/src/components/modals/GoalTemplateModal.tsx" ]; then echo "Error: GoalTemplateModal.tsx file not found" exit 1 fi # Check for the component definition if ! grep -q "export function GoalTemplateModal" "packages/desktop-client/src/components/modals/GoalTemplateModal.tsx"; then echo "Error: GoalTemplateModal component not found in the file" exit 1 fi echo "GoalTemplateModal component exists and has a basic structure" # Display the component for manual review echo "Component content:" cat "packages/desktop-client/src/components/modals/GoalTemplateModal.tsx"Length of output: 3786
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
|
I think this is great! I'm finding the formatting a little hard to read, what do you think about making it a table? It may visually seperate the elements a bit better. E.g. Weekly
Monthly
Longer Term
Schedules
Goals
I also hard to lean forward to read the text here, can we make it a bit bigger?
|
That's fair, I can take a look at making it a table. for the "see more here" bit I copied what is in use for the create local account modal. but i do agree its a bit small here, not sure if it appears smaller due to context or modal size or if it may need to be adjusted on the create account modal as well. but I'll change it here and let you guys decide if it needs to be revisited on the other modal |
|
something like this? with the way the tables renders it feels a bit busy to me. |
|
maybe without the borders. Does this still make it easier to read? |
|
We have components for tables - with styling it would look more like:
You can find examples in ManagePayees.tsx, SelectLinkedAccountsModal.jsx etc. It's just a thought, I'm sure others will weigh in on the UI. |
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
…ge headers to use react-aria-component headings
So I figured out how to do the stylized table, I am however getting this error fro some reason i cant figure out. everything seems to work though. Error Line that's erroring |
That code looks fine to me. You could try restarting your editor in case it's confused or push up your code so I can take a better look. |
Pushed, looks like the type check is pulling the same error |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/modals/GoalTemplateModal.tsx (1 hunks)
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
[failure] 25-25:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 26-26:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 49-49:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 50-50:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 73-73:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 74-74:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 111-111:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 112-112:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 135-135:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 136-136:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx (3)
1-14: LGTM: Imports and component structureThe imports and overall structure of the
GoalTemplateModalcomponent are well-organized. The use ofuseTranslationfor internationalization is a good practice, addressing the requirement mentioned in previous comments.
46-144: LGTM: Consistent structure and comprehensive examplesThe structure and content of the "Monthly", "Longer Term", "Schedules", and "Goals" sections are well-organized and consistent. The examples provided cover a wide range of use cases, which aligns well with the PR objectives of providing a quick reference for goal template syntax.
Note: The type error for the
TableHeadercomponent persists in each section. Please apply the fix suggested in the previous comment to all instances ofTableHeaderin this file.🧰 Tools
🪛 GitHub Check: typecheck
[failure] 49-49:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 50-50:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 73-73:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 74-74:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 111-111:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 112-112:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 135-135:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 136-136:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
1-170: Overall implementation looks good with minor improvements neededThe
GoalTemplateModalcomponent is well-implemented and aligns with the PR objectives. Here's a summary of the strengths and areas for improvement:Strengths:
- Consistent use of internationalization throughout the component.
- Well-structured content providing comprehensive examples of goal templates.
- Use of appropriate React and accessibility components.
Areas for improvement:
- Resolve the type errors for the
TableHeadercomponent as suggested in the earlier comment.- Enhance the external link implementation for better accessibility and security.
Once these minor issues are addressed, the component will be ready for integration into the application.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 25-25:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 26-26:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 49-49:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 50-50:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 73-73:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 74-74:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 111-111:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 112-112:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 135-135:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
[failure] 136-136:
Type '{ name: string; width: string; }' is not assignable to type 'string & Omit<Omit<ViewProps, "ref"> & RefAttributes, "value" | "children"> & { formatter?: (value: string, type?: unknown) => string | Element; ... 10 more ...; privacyFilter?: boolean | PrivacyFilterProps; }'.
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/GoalTemplateModal.tsx
Outdated
Show resolved
Hide resolved
|
Could the "longer-term" section be renamed to "multi month" that way there isn't as much verbiage crossover between that and the "goal" type? |
There was a problem hiding this comment.
I think it's cleaner now, but the modal's looking quite big.
What do people thing about removing the current Headings in favor of table headings to save some space?
e.g.
I think it's still clear what's what from looking at the Table. I'm open to other ideas
Can we shorten this text:
Break down large, less-frequent expenses into manageable monthly expenses
To:
Break down less-frequent expenses into monthly expenses
I don't think it takes away too much and if users want more description they can go to the docs.
|
I like it, cleans things up nicely. I'll make those changes unless anyone has any objections |
This adds a new help menu item to serve as a quick reference for goal template syntax. It includes a few examples for the official docs that should cover a fair number of use cases as well as a link to the official docs.