-
-
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
upcoming schedule setting: move setting to modal #4164
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
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 feature for managing the length of upcoming scheduled transactions. It adds a new 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)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
🧹 Nitpick comments (1)
packages/desktop-client/src/components/schedules/UpcomingLength.tsx (1)
1-1
: Consider enabling TypeScript strict mode.The
@ts-strict-ignore
comment suggests TypeScript strict mode issues. Consider addressing these issues to improve type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4164.md
is excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/Modals.tsx
(2 hunks)packages/desktop-client/src/components/schedules/UpcomingLength.tsx
(1 hunks)packages/desktop-client/src/components/schedules/index.tsx
(4 hunks)packages/desktop-client/src/components/settings/Upcoming.tsx
(0 hunks)packages/loot-core/src/client/state-types/modals.d.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/settings/Upcoming.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (8)
packages/desktop-client/src/components/schedules/UpcomingLength.tsx (2)
13-27
: LGTM! Well-structured custom hook.The
useUpcomingLengthOptions
hook is well-implemented with:
- Clear type definitions using SyncedPrefs
- Internationalized labels using useTranslation
- Logical time period options
29-81
: LGTM! Well-structured modal component.The
UpcomingLength
component is well-implemented with:
- Clear default value handling
- Informative user guidance
- Proper modal structure with header and close button
- Accessible select input for time period selection
packages/desktop-client/src/components/schedules/index.tsx (3)
26-26
: LGTM! Proper feature flag usage.Good use of feature flag to control the rollout of the upcoming length adjustment feature.
43-45
: LGTM! Clean callback implementation.The
onChangeUpcomingLength
callback is well-implemented using useCallback for performance optimization.
127-142
: LGTM! Clean UI implementation.The UI changes are well-structured:
- Proper use of flexbox for layout
- Consistent gap spacing
- Conditional rendering based on feature flag
packages/loot-core/src/client/state-types/modals.d.ts (1)
184-184
: LGTM! Clean type definition.The modal type is correctly defined with null options, consistent with other similar modals in the file.
packages/desktop-client/src/components/Modals.tsx (2)
77-77
: LGTM! Clean import.The import statement is properly placed with other schedule-related imports.
377-379
: LGTM! Clean modal implementation.The modal case is well-implemented:
- Consistent with other modal cases
- Properly passes key prop
- Clean and minimal implementation
a54225e
to
c920e9a
Compare
#3660
From feedback issue:
I've also moved from "Edit" to "Change" terminology, it feels more natural